633638 - Need a way to cancel PlacesUtils::asyncGetBookmarkIds request

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

When I investigated bug 620789, I found the callback function of asyncGetBookmarkIds was invoked several times because previous several requests had not been completed at that time. We need to cancel those requests to obtain correct results.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: cancel previous request if needed

I am not good at English, so comments in this patch might be wrong.

I've addressed marco's comment in bug 620789#367.

Comment 2

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
> I am not good at English, so comments in this patch might be wrong.
(I'd say your English is fine, for what it's worth)

Comment 3

[Marco Bonardo [:mak]]

Thanks I have a couple comments but globally is fine, will post them soon.

In the meanwhile, I'd like to ask blocking for this bug, the point is that there is a high risk we handle notifications for the wrong tab, since tabs and the star are pretty much primary UI there is concrete risk of broken UI.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 511865 [details] [diff] [review]
cancel previous request if needed

>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js

>+
>+    this._pendingStmt = PlacesUtils.asyncGetBookmarkIds(this._uri, function (aItemIds, aURI) {
>+      // For the safty we check that received URI equals this._uri.

I'd just say "Safety check that the bookmarked URI equals the tracked one."

>+      if (this._uri.spec != aURI) {

We should get a nsIURI rather than a spec (see below) then here you want to check aURI.equals(this._uri), it's more efficient.

>+        Components.utils.reportError("PlacesStarButton did not receive current URI");

I'm not sure we want to pollute the Error Console, since looks like it's not going to be hidden by default anymore (I'd approve it, otherwise).
But we should for sure early return here.

>diff --git a/toolkit/components/places/src/PlacesUtils.jsm b/toolkit/components/places/src/PlacesUtils.jsm

>    * @param aCallback
>    *        Function to be called when done.
>-   *        The function will receive an array of itemIds associated to aURI.
>+   *        The function will receive an array of itemIds associated to aURI and
>+   *        the URI spec.

let's just pass out the same uri we got.

>+   * @returns A mozIStoragePendingStatement that can be used to cancel the request.

@return (without the final "s") is the right form

>         if (aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED) {
>-          aCallback.apply(aScope, [this._itemIds]);
>+          aCallback.apply(aScope, [this._itemIds, url]);

just pass-out aURI instead.

nothing maojr but I'd like to do a final pass.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised patch

(In reply to comment #4)
> >+
> >+    this._pendingStmt = PlacesUtils.asyncGetBookmarkIds(this._uri, function (aItemIds, aURI) {
> >+      // For the safty we check that received URI equals this._uri.
> 
> I'd just say "Safety check that the bookmarked URI equals the tracked one."

Done.

> >+      if (this._uri.spec != aURI) {
> 
> We should get a nsIURI rather than a spec (see below) then here you want to
> check aURI.equals(this._uri), it's more efficient.

Done.

> >+        Components.utils.reportError("PlacesStarButton did not receive current URI");
> 
> I'm not sure we want to pollute the Error Console, since looks like it's not
> going to be hidden by default anymore (I'd approve it, otherwise).
> But we should for sure early return here.

Actually I supposed that I did insert return statement there, but forgot! 
I did insert the return statement this time, and left error reporting.

> >diff --git a/toolkit/components/places/src/PlacesUtils.jsm b/toolkit/components/places/src/PlacesUtils.jsm
> 
> >    * @param aCallback
> >    *        Function to be called when done.
> >-   *        The function will receive an array of itemIds associated to aURI.
> >+   *        The function will receive an array of itemIds associated to aURI and
> >+   *        the URI spec.
> 
> let's just pass out the same uri we got.
> 
> >+   * @returns A mozIStoragePendingStatement that can be used to cancel the request.
> 
> @return (without the final "s") is the right form
> 
> >         if (aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED) {
> >-          aCallback.apply(aScope, [this._itemIds]);
> >+          aCallback.apply(aScope, [this._itemIds, url]);
> 
> just pass-out aURI instead.

Done.

Comment 6

[Dietrich Ayala (:dietrich)]

Blocking per comment #3 - high risk of brokenness in primary UI.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 512083 [details] [diff] [review]
Revised patch

>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js

>+    this._pendingStmt = PlacesUtils.asyncGetBookmarkIds(this._uri, function (aItemIds, aURI) {
>+      // Safety check that the bookmarked URI equals the tracked one

nit: end comment with period

>       // Finally re-enable the star.
>       this._ignoreClicks = false;
>+      delete this._pendingStmt;

Hm, I guess we could actually drop _ignoreClicks and just use _pendingStmt as an indication that we are waiting, the only difference would be on about:blank, but the star is not shown in that case, so it doesn't matter much. Could you do that please?
It also requires to change _ignoreClicks to _pendingStmt in browser_bug432599.js and browser_bug581253.js tests

finally, I think in PlacesStarButton.uninit() we want to .cancel any eventual _pendingStmt

We can test this, I'll write a small test and attach it here.

Comment 8

[Marco Bonardo [:mak]]

Attachment: test v1.0

Comment 9

[Mike Shaver (:shaver emeritus)]

The patch has review, let's get it in and see if it helps.

Comment 10

[Marco Bonardo [:mak]]

Yeah, I think Hiroyuki is not around now due to the timezone, I'll update the patch and land it

Comment 11

[Marco Bonardo [:mak]]

So, there is a problem with the fact storage doesn't ensure that a call to mozIStoragePendingStatement.cancel() will produce a onCompletion with REASON_CANCELED, instead it could call back with REASON_FINISHED because it actually finished and was canceled only after that.
This behavior is actually expected according to Storage APIs (I also asked Shawn).
I'm evaluating if I should wrap mozIStoragePendingStatement in a object with just a cancel() method to properly callback only when we didn't cancel.

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch with fixed comments

Comment 13

[Marco Bonardo [:mak]]

Attachment: test v1.1

Comment 14

[Marco Bonardo [:mak]]

Attachment: patch with fixed comments

fix patch author

Comment 15

[Marco Bonardo [:mak]]

Attachment: wrap statement for REASON_CANCELED

This wraps the statement execution so that invoking cancel() on the returned object will always cause a REASON_CANCELED (and thus skip original callback invokation).
This part is needed by my last test that checks that cancel() does actually avoid invoking the original callback, so this is tested by it as well.

Comment 16

[Marco Bonardo [:mak]]

(In reply to comment #15)
> Created attachment 512245 [details] [diff] [review]
> wrap statement for REASON_CANCELED

actually, the second part of the javadoc of the wrapper is bogus, I had removed it locally but looks like I forgot to qrefresh.

Comment 17

[Shawn Wilsher :sdwilsh]

Comment on attachment 512245 [details] [diff] [review]
wrap statement for REASON_CANCELED

>+function AsyncStatementWithSyncCancel(aStmt) {
nit: what about AsyncStatementCancelTracker?

r=sdwilsh

Comment 18

[Marco Bonardo [:mak]]

Attachment: part2: wrap statement for REASON_CANCELED

decided for AsyncStatementCancelWrapper

The bug should be ready to go, just waiting for the tree to be in a good shape.

Comment 19

[Marco Bonardo [:mak]]

Attachment: part1: provide a way to cancel asyncGetBookmarkIds

while running b-c tests locally I've noticed a glitch.  This makes sure when we cancel the pending statement we also delete it.

Comment 20

[Marco Bonardo [:mak]]

part1: http://hg.mozilla.org/mozilla-central/rev/1422443b6c15
part2: http://hg.mozilla.org/mozilla-central/rev/0d94b660ba28
test: http://hg.mozilla.org/mozilla-central/rev/9d7966447a9b