Closed Bug 633638 Opened 9 years ago Closed 9 years ago

Need a way to cancel PlacesUtils::asyncGetBookmarkIds request

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b12
Tracking Status
blocking2.0 --- final+

People

(Reporter: hiro, Assigned: hiro)

References

Details

(Whiteboard: [hardblocker][has patch])

Attachments

(3 files, 6 obsolete files)

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.
I am not good at English, so comments in this patch might be wrong.

I've addressed marco's comment in bug 620789#367.
Attachment #511865 - Flags: review?(mak77)
(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)
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.
blocking2.0: --- → ?
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
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.
Attachment #511865 - Flags: review?(mak77) → review-
Attached patch Revised patch (obsolete) — Splinter Review
(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.
Attachment #511865 - Attachment is obsolete: true
Attachment #512083 - Flags: review?(mak77)
Blocking per comment #3 - high risk of brokenness in primary UI.
blocking2.0: ? → final+
Whiteboard: [hardblocker][has patch][needs review marco]
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.
Attachment #512083 - Flags: review?(mak77) → review+
Attached patch test v1.0 (obsolete) — Splinter Review
Attachment #512153 - Flags: review?(dietrich)
Attachment #512153 - Flags: review?(dietrich) → review+
The patch has review, let's get it in and see if it helps.
Whiteboard: [hardblocker][has patch][needs review marco] → [hardblocker][has patch]
Yeah, I think Hiroyuki is not around now due to the timezone, I'll update the patch and land it
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.
Attached patch patch with fixed comments (obsolete) — Splinter Review
Attachment #512083 - Attachment is obsolete: true
Attached patch test v1.1Splinter Review
Attachment #512153 - Attachment is obsolete: true
Attached patch patch with fixed comments (obsolete) — Splinter Review
fix patch author
Attachment #512239 - Attachment is obsolete: true
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.
Attachment #512245 - Flags: review?(sdwilsh)
(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 on attachment 512245 [details] [diff] [review]
wrap statement for REASON_CANCELED

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

r=sdwilsh
Attachment #512245 - Flags: review?(sdwilsh) → review+
decided for AsyncStatementCancelWrapper

The bug should be ready to go, just waiting for the tree to be in a good shape.
Attachment #512245 - Attachment is obsolete: true
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][can land]
while running b-c tests locally I've noticed a glitch.  This makes sure when we cancel the pending statement we also delete it.
Attachment #512242 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
Whiteboard: [hardblocker][has patch][can land] → [hardblocker][has patch]
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.