887865 - Use a mozIStorageAsyncConnection in for GetIsVisitedStatement

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

History.cpp clones the Places connection on main-thread, since it's used for link coloring it may cause jank on first page load, so we should convert it to clone asynchronously.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1

Comment 2

[Marco Bonardo [:mak]]

the patch is mostly fine, but it's causing
Assertion failure: !mAsyncExecutionThread (AsyncClose has not been invoked on this connection!)

Since we asyncClose both connections I suspect there's something else involved, off-hand I can't tell if it's the main connection or the clone that causes it. Will need some debugger work.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8470029 [details] [diff] [review]
patch v1

Clearing request cause I must refactor this a little bit.

Basically the statement request can happen multiple times before the first request calls back, so I need a way to enqueue callbacks to avoid creating multiple clones.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v2

OK, I had to refactor all the isvisited statement handling, but I think the result is nicer.

This is currently running on tryserver to check I catched everything:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e4f9e331bf3f

Comment 5

[Asaf Romano (gone)]

Comment on attachment 8471020 [details] [diff] [review]
patch v2

Review of attachment 8471020 [details] [diff] [review]:
-----------------------------------------------------------------

Just some nits.

::: toolkit/components/places/History.cpp
@@ +437,1 @@
>  public:

Move that under |public:|, and please add a new line between that to the next method.

@@ +479,5 @@
> +  NS_IMETHOD Complete(nsresult aResult, nsISupports* aStatement)
> +  {
> +    if (NS_FAILED(aResult)) {
> +      return aResult;
> +    }

It should be mentioned somewhere here that the return value of this method actually matters: that is, that it's not that just some XPCOM service calls this method, but that we call this method internally and check its return value.

You do an if (NS_FAILED..) check here, but few line later you use NS_ENSURE_SUCCESS. Any reason for that?

@@ +558,5 @@
>    , mIsVisited(aIsVisited)
>    {
>    }
>  
> +  ~VisitedQuery()

Either virtual, or MOZ_FINAL the class.

@@ +2032,5 @@
> +    DebugOnly<nsresult> rv = aDBConn->AsyncClone(true, this);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> +  }
> +
> +  NS_IMETHODIMP Complete(nsresult aStatus, nsISupports* aConnection) {

NS_IMETHOD

@@ +2034,5 @@
> +  }
> +
> +  NS_IMETHODIMP Complete(nsresult aStatus, nsISupports* aConnection) {
> +    if (NS_FAILED(aStatus))
> +      return aStatus;

And here I guess it doesn't really matter, does it? If not, just return NS_OK.

@@ +2047,5 @@
> +      ),  getter_AddRefs(mIsVisitedStatement));
> +      MOZ_ASSERT(mIsVisitedStatement);
> +      for (int32_t i = 0; i < mIsVisitedCallbacks.Count(); ++i) {
> +        DebugOnly<nsresult> rv;
> +        if (mIsVisitedStatement) {

Let's do the check outside, along the assert, save the status and pass it to Complete, unconditionally.

@@ +2063,5 @@
> +
> +  void GetIsVisitedStatement(mozIStorageCompletionCallback* aCallback)
> +  {
> +    if (mIsVisitedStatement) {
> +      aCallback->Complete(NS_OK, mIsVisitedStatement);

(void)

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v3

Addressed comments.

Comment 7

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/fx-team/rev/b3877ffb2c4a

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b3877ffb2c4a