Closed Bug 880226 Opened 11 years ago Closed 11 years ago

Intermittent browser_bug386835.js | Tab 3 should have zoomed as it was loading in the background - Got 1, expected 1.100000023841858 (and two more)

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 25
Tracking Flags:
Tracking Status
firefox24 --- fixed
firefox25 --- fixed

People

(Reporter: emorley, Assigned: adw)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 5 obsolete files)

tokens again 2
24.07 KB, patch
Details | Diff | Splinter Review 
Regressed by bug 555120 ?

Rev5 MacOSX Mountain Lion 10.8 mozilla-inbound opt test mochitest-browser-chrome on 2013-06-05 19:23:38 PDT for push b2c600be7e90

slave: talos-mtnlion-r5-036

https://tbpl.mozilla.org/php/getParsedLog.php?id=23841469&tree=Mozilla-Inbound

{
19:25:45     INFO -  TEST-START | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js
19:25:46     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.org/browser/browser/base/content/test/dummy_page.html" line: 0}]
19:25:46     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.org/browser/browser/base/content/test/dummy_page.html" line: 0}]
19:25:46     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Initial zoom of tab 1 should be 1
19:25:46     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Initial zoom of tab 2 should be 1
19:25:46     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Initial zoom of tab 3 should be 1
19:25:46     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | New zoom for tab 1 should be greater than 1
19:25:46     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Zooming tab 1 should not affect tab 2
19:25:46     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Zooming tab 1 should not affect tab 3
19:25:46     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.org/browser/browser/base/content/test/dummy_page.html" line: 0}]
19:25:46     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Tab 1 should still be zoomed
19:25:46     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Tab 2 should still not be affected
19:25:46  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Tab 3 should have zoomed as it was loading in the background - Got 1, expected 1.100000023841858
19:25:46     INFO -  Stack trace:
19:25:46     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 536
19:25:46     INFO -      JS frame :: chrome://mochitests/content/browser/browser/base/content/test/head.js :: zoomTest :: line 329
19:25:46     INFO -      JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js :: <TOP_LEVEL> :: line 45
19:25:46     INFO -      JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 192
19:25:46     INFO -      JS frame :: resource://gre/modules/Task.jsm :: TaskImpl :: line 163
19:25:46     INFO -      JS frame :: resource://gre/modules/Task.jsm :: Task_spawn :: line 135
19:25:46     INFO -      JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js :: thirdPageLoaded :: line 42
19:25:46     INFO -      JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
19:25:46     INFO -      JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
19:25:46     INFO -      JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
19:25:46     INFO -      JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 220
19:25:46     INFO -      JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
19:25:46     INFO -      JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
19:25:46     INFO -      JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
19:25:46     INFO -      JS frame :: chrome://mochitests/content/browser/browser/base/content/test/head.js :: <TOP_LEVEL> :: line 307
19:25:46     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
19:25:46     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Tab 1 should still be zoomed
19:25:46  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Tab 2 should be zoomed now - Got 1, expected 1.100000023841858
19:25:46     INFO -  Stack trace:
19:25:46     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 536
19:25:46     INFO -      JS frame :: chrome://mochitests/content/browser/browser/base/content/test/head.js :: zoomTest :: line 329
19:25:46     INFO -      JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js :: <TOP_LEVEL> :: line 50
19:25:46     INFO -      JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 192
19:25:46     INFO -      JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
19:25:46     INFO -      JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
19:25:46     INFO -      JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
19:25:46     INFO -      JS frame :: chrome://mochitests/content/browser/browser/base/content/test/head.js :: obs :: line 293
19:25:46     INFO -      JS frame :: chrome://browser/content/browser.js :: FullZoom__notifyOnLocationChange/< :: line 1845
19:25:46     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
}
(In reply to Ed Morley [:edmorley UTC+1] from comment #0)
> Regressed by bug 555120 ?

Almost certainly.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Sigh.  I think this is what's happening in the failing runs:

1. The selected tab's page is P.
2. The test enlarges its zoom.
3. The test calls FullZoomHelper.load to load P in a background tab.
4. Because FullZoomHelper.load is called for a background tab, it
   skips waiting for the FullZoom notification it normally listens
   for before returning control to the caller.
5. The test continues and checks the background tab's zoom, but
   FullZoom hasn't changed it yet: "Got 1, expected > 1."

Step 4 is why the test fails, but there's another problem.  FullZoomHelper.load skips waiting for background tabs because FullZoom.onLocationChange doesn't update the zoom of background tabs -- but actually this is messed up.  It does update the zoom of background tabs, but only if their domains happen to match the domain of the selected tab.

Basically FullZoom doesn't handle background loads correctly:

(1) _isStateCurrent needs access to the same browser that was passed to _getState.  Otherwise it ends up comparing the domain of the browser whose zoom needs changing to the domain of the currently selected browser.  It should be comparing domains of the same browser, the browser whose zoom needs changing, before and after the async op.

(2) It's not right to have one global _zoomChangeToken.  If an async zoom change starts on a background tab because it loaded, but then the zoom is changed on the selected tab, the selected tab's zoom shouldn't clobber the background change if their domains are different.  You need a zoomChangeToken per domain.
(In reply to Drew Willcoxon :adw from comment #16)
> You need a zoomChangeToken per domain.

Hmm, do you mean per browser?

AIUI:
a) the token ensures that for a given browser, the latest zoom change always wins (i.e. ensures changes are processed in the order that they were requested)
b) the domain check ensures that for a given browser, the zoom change only applies if the page hasn't since been navigated away

For a), I kind of would have expected ordering could be preserved by content prefs somehow (does the storage API not allow guaranteeing the order of query results? or is caching getting in the way of that?). Assuming it can't be, a token seems like a good solution.

For b), it seems like things would be simpler (and more efficient) if we could cancel pending content prefs queries for a given browser on navigation start, or something like that. IIRC we talked about that and you said it was hard to do, though.
I'm having a hard time answering because I've been thinking about this all day and now my brain is mush.

Storage does queue requests, and CPS uses Storage.  But you can't really rely on that when changing zoom because of reduce() and enlarge() because they're relative to some current zoom, not absolute zoom values.

Say you want to implement enlarge by writing the new content pref, and when the write is done, updating the visual zoom.  But what value should you write to CPS?  You can't base it on the current zoom level, because there may be many writes to CPS queued up.  The new enlarged zoom level has to be relative to all those changes.  So you have to write a value that you can't possibly know yet.

So then you think, I'll just update the visual zoom immediately and then schedule the pref write.  The immediate visual zoom changes and the CPS writes are interleaved, but that's OK because each is internally ordered, and they don't affect each other.  Except they do affect each other, because sometimes you have to read from CPS and use that value to set the visual zoom.  So you run into situations like a page loads and its pref is fetched from CPS, but before that finishes the user enlarges the zoom.  You can't ignore the enlarge since it came after the load.  If you don't ignore the fetch, then it clobbers the enlarge, and then you have to do the enlarge at some later point, like when its pref write completes, but you're updating visual zoom immediately, not when pref writes complete, remember?

So now you have to ignore the fetch.  What if it's a background tab that loads rather than the selected tab, its domain is the same as the selected tab's, and site-specific zoom is turned on?  Should the enlarge in the selected tab cause the background tab's pref fetch to be ignored too?  What if the user quickly switches to that background tab after enlarging the first one?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #30)
> For b), it seems like things would be simpler (and more efficient) if we
> could cancel pending content prefs queries for a given browser on navigation
> start, or something like that.

But if you navigate away and ultimately end up on a page with the same domain, and site-specific zoom is turned on, then the zoom should apply to the new page.

In general, if some async thing starts on a page with domain D and finishes on a page with domain D, then that async thing should affect the final page if site-specific zoom is turned on.  If it's turned off, then the async thing should only apply if it's the same browser.

Or maybe that's not right at all since when you switch tabs onLocationChange is called and updates the tab's zoom.
Attached patch part 1: nsIContentPrefPending (obsolete) — — Splinter Review 
This adds an nsIContentPrefPending interface that can be used to cancel pending CPS requests, like mozIStoragePendingStatement in Storage.  The FullZoom patch I'm about to post uses it.  This is equivalent to the token approach I was using before, but now all consumers can benefit.  I guess it's slightly better than the tokens, but after implementing and using it I'm not 100% convinced it's really worth it.  So please don't feel you have to prefer this big change over the tokens just because I did all the work of implementing it.

Originally I exposed mozIStoragePendingStatement directly, but Gavin thought it was a little better to not expose that implementation detail, which I agree with, although neither of us were 100% about that either.
Attachment #762453 - Flags: review?(mak77)
Attached patch part 2: FullZoom (obsolete) — — Splinter Review 
As I mentioned nsIContentPrefPending is equivalent to the tokens, so that's not the big deal here.  The big deals are:

* associating async zoom accesses with specific browsers
* canceling async zoom accesses for a browser when its location changes and
  its zoom is updated
* onLocationChange calling _notifyOnLocationChange in all cases (it previously
  did not when getByDomainAndName finishes but the state is no longer current)
* and the change in head.js to hook into the previous point.

https://tbpl.mozilla.org/?tree=Try&rev=66fa775c7dd0
Attachment #762455 - Flags: review?(mak77)
Comment on attachment 762455 [details] [diff] [review]
part 2: FullZoom

Try revealed a problem that I'm fixing.  (Cancelables can end up getting canceled more than once, which agitates mozIStoragePendingStatement.)  I'll post a new patch when try reveals no more problems.

Review on the other patch can go ahead still.
Attachment #762455 - Flags: review?(mak77)
Attached patch part 2: FullZoom (obsolete) — — Splinter Review 
Small change to allow pending zoom access objects to be canceled more than once.  Comment 106 applies to this too.

There are many failures on try, but none look related: https://tbpl.mozilla.org/?tree=Try&rev=3bf5c33d1a1c
Attachment #762455 - Attachment is obsolete: true
Attachment #763059 - Flags: review?(mak77)
My previous patch made this test fail.  I'm not 100% sure why -- or how it passed after the original async FullZoom bug.  I think it has to do with timings of accesses to CPS's storage, which accesses are queued up and serviced at what time.

The previous patch made it fail after it enlarges a tab, switches away, and then switches back.  When the test then checked the zoom, it was still 1 when it should have been > 1.  This patch makes it carefully wait whenever it selects and loads tabs, like all the other tests I updated in the original async FullZoom bug.
Attachment #763061 - Flags: review?(mak77)
Disabled temporarily whilst waiting for review to reduce the noise, since my spidey sense says it may be a while... ;-)
https://hg.mozilla.org/integration/mozilla-inbound/rev/84e4bef463e5
Whiteboard: [test disabled][leave open]
Are you sure we can rely on pendingStatement cancelation? As far as I know it's really not something I'd rely on, cause it may happen at any time, the statement may have already selected and returned all of the results, maybe even completed, when you cancel it. It's hard to rely on something that depends on a thread scheduler. iirc the idea behind Cancel() was originally implemented to allow to stop fetching results while fetching many results, most for performance reasons that for other, indeed after you invoke it, you may even still get some results.

Now, the issues is quite complicated and I'm pretty sure you have a much deeper knowledge of the issue than me, I could barely follow all of the description above.
I was wondering if we couldn't use a queue of changes here, where before returning from any operation we walk up the queue and see if next requests may clobber, if so enqueue the callback, there may be a queue for each domain and you could clean it up with a reasonable count limit.

Though, if you think your implementation does not rely on specific schedule of Cancel() calls it may be fine to proceed with it.
or well, once an operation completes is may remove everything in queue before itself, a monotonic token to identify the operation could help.
(In reply to Marco Bonardo [:mak] from comment #173)
> Are you sure we can rely on pendingStatement cancelation? As far as I know
> it's really not something I'd rely on, cause it may happen at any time, the
> statement may have already selected and returned all of the results, maybe
> even completed, when you cancel it.

That's true.  My FullZoom patch only cancels getters, though, and it doesn't care if any results were actually gotten.  It uses cancel() only as a way to mark getter requests so that it can ignore them when they finish.

This is a good point you make.  We should probably forget this cancelable patch.  The CPS API shouldn't lead consumers to believe that requests can actually be canceled before they affect the database.

> I was wondering if we couldn't use a queue of changes here, where before
> returning from any operation we walk up the queue and see if next requests
> may clobber, if so enqueue the callback, there may be a queue for each
> domain and you could clean it up with a reasonable count limit.

Hmm, I'll have to think about that.  My initial impression is that queues sound unnecessary when you could simply keep a monotonic token per domain.  Right?  Simply store the current maximum token in a queue (i.e., the token of the operation at the end of the queue) and then you don't actually need the queue at all.

Thanks for helping me with this!
Comment on attachment 762453 [details] [diff] [review]
part 1: nsIContentPrefPending

>diff --git a/content/html/content/src/HTMLInputElement.cpp b/content/html/content/src/HTMLInputElement.cpp
>--- a/content/html/content/src/HTMLInputElement.cpp
>+++ b/content/html/content/src/HTMLInputElement.cpp
>@@ -540,17 +540,18 @@ UploadLastDir::FetchDirectoryAndDisplayP
>     prefCallback->HandleCompletion(nsIContentPrefCallback2::COMPLETE_ERROR);
>     return NS_OK;
>   }
> 
>   nsAutoCString cstrSpec;
>   docURI->GetSpec(cstrSpec);
>   NS_ConvertUTF8toUTF16 spec(cstrSpec);
> 
>-  contentPrefService->GetByDomainAndName(spec, CPS_PREF_NAME, loadContext, prefCallback);
>+  nsCOMPtr<nsIContentPrefPending> pending;
>+  contentPrefService->GetByDomainAndName(spec, CPS_PREF_NAME, loadContext, prefCallback, getter_AddRefs(pending));
>   return NS_OK;
> }
> 
> nsresult
> UploadLastDir::StoreLastUsedDirectory(nsIDocument* aDoc, nsIDOMFile* aDomFile)
> {
>   NS_PRECONDITION(aDoc, "aDoc is null");
>   NS_PRECONDITION(aDomFile, "aDomFile is null");
>@@ -592,27 +593,31 @@ UploadLastDir::StoreLastUsedDirectory(ns
>     return NS_OK;
>   nsCOMPtr<nsIWritableVariant> prefValue = do_CreateInstance(NS_VARIANT_CONTRACTID);
>   if (!prefValue)
>     return NS_ERROR_OUT_OF_MEMORY;
>   prefValue->SetAsAString(unicodePath);
> 
>   nsCOMPtr<nsISupports> container = aDoc->GetContainer();
>   nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(container);
>-  return contentPrefService->Set(spec, CPS_PREF_NAME, prefValue, loadContext, nullptr);
>+  nsCOMPtr<nsIContentPrefPending> pending;
>+  return contentPrefService->Set(spec, CPS_PREF_NAME, prefValue, loadContext, nullptr, getter_AddRefs(pending));
> }
> 
> NS_IMETHODIMP
> UploadLastDir::Observe(nsISupports* aSubject, char const* aTopic, PRUnichar const* aData)
> {
>   if (strcmp(aTopic, "browser:purge-session-history") == 0) {
>     nsCOMPtr<nsIContentPrefService2> contentPrefService =
>       do_GetService(NS_CONTENT_PREF_SERVICE_CONTRACTID);
>-    if (contentPrefService)
>-      contentPrefService->RemoveByName(CPS_PREF_NAME, nullptr, nullptr);
>+    if (contentPrefService) {
>+      nsCOMPtr<nsIContentPrefPending> pending;
>+      contentPrefService->RemoveByName(CPS_PREF_NAME, nullptr, nullptr,
>+                                       getter_AddRefs(pending));
>+    }
>   }
>   return NS_OK;
> }
> 
> #ifdef ACCESSIBILITY
> //Helper method
> static nsresult FireEventForAccessibility(nsIDOMHTMLInputElement* aTarget,
>                                           nsPresContext* aPresContext,
>diff --git a/dom/interfaces/base/nsIContentPrefService2.idl b/dom/interfaces/base/nsIContentPrefService2.idl
>--- a/dom/interfaces/base/nsIContentPrefService2.idl
>+++ b/dom/interfaces/base/nsIContentPrefService2.idl
>@@ -4,16 +4,17 @@
> 
> #include "nsISupports.idl"
> 
> interface nsIVariant;
> interface nsIContentPrefObserver;
> interface nsIContentPrefCallback2;
> interface nsILoadContext;
> interface nsIContentPref;
>+interface nsIContentPrefPending;
> 
> /**
>  * Content Preferences
>  *
>  * Content preferences allow the application to associate arbitrary data, or
>  * "preferences", with specific domains, or web "content".  Specifically, a
>  * content preference is a structure with three values: a domain with which the
>  * preference is associated, a name that identifies the preference within its
>@@ -62,59 +63,62 @@ interface nsIContentPref;
>  * The methods of callback objects are always called asynchronously.
>  *
>  * Observers are called after callbacks are called, but they are called in the
>  * same turn of the event loop as callbacks.
>  *
>  * See nsIContentPrefCallback2 below for more information about callbacks.
>  */
> 
>-[scriptable, uuid(133608c7-f812-41ca-bc1c-62a4eb95e52a)]
>+[scriptable, uuid(13de9efd-5ef0-4412-a4b5-66e3940c6f5b)]
> interface nsIContentPrefService2 : nsISupports
> {
>   /**
>    * Gets the preference with the given domain and name.
>    *
>    * @param domain    The preference's domain.
>    * @param name      The preference's name.
>    * @param context   The private-browsing context, if any.
>    * @param callback  handleResult is called once unless no such preference
>    *                  exists, in which case handleResult is not called at all.
>+   * @return  A pending object that can be used to cancel the request.
>    */
>-  void getByDomainAndName(in AString domain,
>-                          in AString name,
>-                          in nsILoadContext context,
>-                          in nsIContentPrefCallback2 callback);
>+  nsIContentPrefPending getByDomainAndName(in AString domain,
>+                                           in AString name,
>+                                           in nsILoadContext context,
>+                                           in nsIContentPrefCallback2 callback);
> 
>   /**
>    * Gets all preferences with the given name whose domains are either the same
>    * as or subdomains of the given domain.
>    *
>    * @param domain    The preferences' domain.
>    * @param name      The preferences' name.
>    * @param context   The private-browsing context, if any.
>    * @param callback  handleResult is called once for each preference.  If no
>    *                  such preferences exist, handleResult is not called at all.
>+   * @return  A pending object that can be used to cancel the request.
>    */
>-  void getBySubdomainAndName(in AString domain,
>-                             in AString name,
>-                             in nsILoadContext context,
>-                             in nsIContentPrefCallback2 callback);
>+  nsIContentPrefPending getBySubdomainAndName(in AString domain,
>+                                              in AString name,
>+                                              in nsILoadContext context,
>+                                              in nsIContentPrefCallback2 callback);
> 
>   /**
>    * Gets the preference with no domain and the given name.
>    *
>    * @param name      The preference's name.
>    * @param context   The private-browsing context, if any.
>    * @param callback  handleResult is called once unless no such preference
>    *                  exists, in which case handleResult is not called at all.
>+   * @return  A pending object that can be used to cancel the request.
>    */
>-  void getGlobal(in AString name,
>-                 in nsILoadContext context,
>-                 in nsIContentPrefCallback2 callback);
>+  nsIContentPrefPending getGlobal(in AString name,
>+                                  in nsILoadContext context,
>+                                  in nsIContentPrefCallback2 callback);
> 
>   /**
>    * Synchronously retrieves from the in-memory cache the preference with the
>    * given domain and name.
>    *
>    * In addition to caching preference values, the cache also keeps track of
>    * preferences that are known not to exist.  If the preference is known not to
>    * exist, the value attribute of the returned object will be undefined
>@@ -177,129 +181,139 @@ interface nsIContentPrefService2 : nsISu
>    * Sets a preference.
>    *
>    * @param domain    The preference's domain.
>    * @param name      The preference's name.
>    * @param value     The preference's value.
>    * @param context   The private-browsing context, if any.
>    * @param callback  handleCompletion is called when the preference has been
>    *                  stored.
>+   * @return  A pending object that can be used to cancel the request.
>    */
>-  void set(in AString domain,
>-           in AString name,
>-           in nsIVariant value,
>-           in nsILoadContext context,
>-           [optional] in nsIContentPrefCallback2 callback);
>+  nsIContentPrefPending set(in AString domain,
>+                            in AString name,
>+                            in nsIVariant value,
>+                            in nsILoadContext context,
>+                            [optional] in nsIContentPrefCallback2 callback);
> 
>   /**
>    * Sets a preference with no domain.
>    *
>    * @param name      The preference's name.
>    * @param value     The preference's value.
>    * @param context   The private-browsing context, if any.
>    * @param callback  handleCompletion is called when the preference has been
>    *                  stored.
>+   * @return  A pending object that can be used to cancel the request.
>    */
>-  void setGlobal(in AString name,
>-                 in nsIVariant value,
>-                 in nsILoadContext context,
>-                 [optional] in nsIContentPrefCallback2 callback);
>+  nsIContentPrefPending setGlobal(in AString name,
>+                                  in nsIVariant value,
>+                                  in nsILoadContext context,
>+                                  [optional] in nsIContentPrefCallback2 callback);
> 
>   /**
>    * Removes the preference with the given domain and name.
>    *
>    * @param domain    The preference's domain.
>    * @param name      The preference's name.
>    * @param context   The private-browsing context, if any.
>    * @param callback  handleCompletion is called when the operation completes.
>+   * @return  A pending object that can be used to cancel the request.
>    */
>-  void removeByDomainAndName(in AString domain,
>-                             in AString name,
>-                             in nsILoadContext context,
>-                             [optional] in nsIContentPrefCallback2 callback);
>+  nsIContentPrefPending removeByDomainAndName(in AString domain,
>+                                              in AString name,
>+                                              in nsILoadContext context,
>+                                              [optional] in nsIContentPrefCallback2 callback);
> 
>   /**
>    * Removes all the preferences with the given name whose domains are either
>    * the same as or subdomains of the given domain.
>    *
>    * @param domain    The preferences' domain.
>    * @param name      The preferences' name.
>    * @param context   The private-browsing context, if any.
>    * @param callback  handleCompletion is called when the operation completes.
>+   * @return  A pending object that can be used to cancel the request.
>    */
>-  void removeBySubdomainAndName(in AString domain,
>-                                in AString name,
>-                                in nsILoadContext context,
>-                                [optional] in nsIContentPrefCallback2 callback);
>+  nsIContentPrefPending removeBySubdomainAndName(in AString domain,
>+                                                 in AString name,
>+                                                 in nsILoadContext context,
>+                                                 [optional] in nsIContentPrefCallback2 callback);
> 
>   /**
>    * Removes the preference with no domain and the given name.
>    *
>    * @param name      The preference's name.
>    * @param context   The private-browsing context, if any.
>    * @param callback  handleCompletion is called when the operation completes.
>+   * @return  A pending object that can be used to cancel the request.
>    */
>-  void removeGlobal(in AString name,
>-                    in nsILoadContext context,
>-                    [optional] in nsIContentPrefCallback2 callback);
>+  nsIContentPrefPending removeGlobal(in AString name,
>+                                     in nsILoadContext context,
>+                                     [optional] in nsIContentPrefCallback2 callback);
> 
>   /**
>    * Removes all preferences with the given domain.
>    *
>    * @param domain    The preferences' domain.
>    * @param context   The private-browsing context, if any.
>    * @param callback  handleCompletion is called when the operation completes.
>+   * @return  A pending object that can be used to cancel the request.
>    */
>-  void removeByDomain(in AString domain,
>-                      in nsILoadContext context,
>-                      [optional] in nsIContentPrefCallback2 callback);
>+  nsIContentPrefPending removeByDomain(in AString domain,
>+                                       in nsILoadContext context,
>+                                       [optional] in nsIContentPrefCallback2 callback);
> 
>   /**
>    * Removes all preferences whose domains are either the same as or subdomains
>    * of the given domain.
>    *
>    * @param domain    The preferences' domain.
>    * @param context   The private-browsing context, if any.
>    * @param callback  handleCompletion is called when the operation completes.
>+   * @return  A pending object that can be used to cancel the request.
>    */
>-  void removeBySubdomain(in AString domain,
>-                         in nsILoadContext context,
>-                         [optional] in nsIContentPrefCallback2 callback);
>+  nsIContentPrefPending removeBySubdomain(in AString domain,
>+                                          in nsILoadContext context,
>+                                          [optional] in nsIContentPrefCallback2 callback);
> 
>   /**
>    * Removes all preferences with the given name regardless of domain, including
>    * global preferences with the given name.
>    *
>    * @param name      The preferences' name.
>    * @param context   The private-browsing context, if any.
>    * @param callback  handleCompletion is called when the operation completes.
>+   * @return  A pending object that can be used to cancel the request.
>    */
>-  void removeByName(in AString name,
>-                    in nsILoadContext context,
>-                    [optional] in nsIContentPrefCallback2 callback);
>+  nsIContentPrefPending removeByName(in AString name,
>+                                     in nsILoadContext context,
>+                                     [optional] in nsIContentPrefCallback2 callback);
> 
>   /**
>    * Removes all non-global preferences -- in other words, all preferences that
>    * have a domain.
>    *
>    * @param context   The private-browsing context, if any.
>    * @param callback  handleCompletion is called when the operation completes.
>+   * @return  A pending object that can be used to cancel the request.
>    */
>-  void removeAllDomains(in nsILoadContext context,
>-                        [optional] in nsIContentPrefCallback2 callback);
>+  nsIContentPrefPending removeAllDomains(in nsILoadContext context,
>+                                         [optional] in nsIContentPrefCallback2 callback);
> 
>   /**
>    * Removes all global preferences -- in other words, all preferences that have
>    * no domain.
>    *
>    * @param context   The private-browsing context, if any.
>    * @param callback  handleCompletion is called when the operation completes.
>+   * @return  A pending object that can be used to cancel the request.
>    */
>-  void removeAllGlobals(in nsILoadContext context,
>-                        [optional] in nsIContentPrefCallback2 callback);
>+  nsIContentPrefPending removeAllGlobals(in nsILoadContext context,
>+                                         [optional] in nsIContentPrefCallback2 callback);
> 
>   /**
>    * Registers an observer that will be notified whenever a preference with the
>    * given name is set or removed.
>    *
>    * When a set or remove method is called, observers are called after the set
>    * or removal completes and after the method's callback is called, and they
>    * are called in the same turn of the event loop as the callback.
>@@ -334,17 +348,17 @@ interface nsIContentPrefService2 : nsISu
>    *             returned.  Otherwise, the string itself is returned.
>    */
>   AString extractDomain(in AString str);
> };
> 
> /**
>  * The callback used by the above methods.
>  */
>-[scriptable, uuid(1a12cf41-79e8-4d0f-9899-2f7b27c5d9a1)]
>+[scriptable, uuid(a72cee89-9546-438e-9e75-513bc46485d8)]
> interface nsIContentPrefCallback2 : nsISupports
> {
>   /**
>    * For the retrieval methods, this is called once for each retrieved
>    * preference.  It is not called for other methods.
>    *
>    * @param pref  The retrieved preference.
>    */
>@@ -365,16 +379,28 @@ interface nsIContentPrefCallback2 : nsIS
>    *
>    * @param reason  One of the COMPLETE_* values indicating the manner in which
>    *                the method completed.
>    */
>   void handleCompletion(in unsigned short reason);
> 
>   const unsigned short COMPLETE_OK = 0;
>   const unsigned short COMPLETE_ERROR = 1;
>+  const unsigned short COMPLETE_CANCELED = 2;
>+};
>+
>+[scriptable, uuid(1df07db6-5bed-467b-a08b-3a0f13e03f36)]
>+interface nsIContentPrefPending : nsISupports
>+{
>+  /**
>+   * Cancels this object's associated request.  If a callback was passed to the
>+   * request, then the callback's handleCompletion method will be called with
>+   * reason COMPLETE_CANCELED.
>+   */
>+  void cancel();
> };
> 
> [scriptable, function, uuid(9f24948d-24b5-4b1b-b554-7dbd58c1d792)]
> interface nsIContentPref : nsISupports
> {
>   readonly attribute AString domain;
>   readonly attribute AString name;
>   readonly attribute nsIVariant value;
>diff --git a/editor/composer/src/nsEditorSpellCheck.cpp b/editor/composer/src/nsEditorSpellCheck.cpp
>--- a/editor/composer/src/nsEditorSpellCheck.cpp
>+++ b/editor/composer/src/nsEditorSpellCheck.cpp
>@@ -164,19 +164,20 @@ DictionaryFetcher::Fetch(nsIEditor* aEdi
>   rv = docUri->GetSpec(docUriSpec);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   nsCOMPtr<nsIContentPrefService2> contentPrefService =
>     do_GetService(NS_CONTENT_PREF_SERVICE_CONTRACTID);
>   NS_ENSURE_TRUE(contentPrefService, NS_ERROR_NOT_AVAILABLE);
> 
>   nsCOMPtr<nsILoadContext> loadContext = GetLoadContext(aEditor);
>+  nsCOMPtr<nsIContentPrefPending> pending;
>   rv = contentPrefService->GetByDomainAndName(NS_ConvertUTF8toUTF16(docUriSpec),
>                                               CPS_PREF_NAME, loadContext,
>-                                              this);
>+                                              this, getter_AddRefs(pending));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return NS_OK;
> }
> 
> /**
>  * Stores the current dictionary for aEditor's document URL.
>  */
>@@ -199,19 +200,20 @@ StoreCurrentDictionary(nsIEditor* aEdito
>   NS_ENSURE_TRUE(prefValue, NS_ERROR_OUT_OF_MEMORY);
>   prefValue->SetAsAString(aDictionary);
> 
>   nsCOMPtr<nsIContentPrefService2> contentPrefService =
>     do_GetService(NS_CONTENT_PREF_SERVICE_CONTRACTID);
>   NS_ENSURE_TRUE(contentPrefService, NS_ERROR_NOT_INITIALIZED);
> 
>   nsCOMPtr<nsILoadContext> loadContext = GetLoadContext(aEditor);
>+  nsCOMPtr<nsIContentPrefPending> pending;
>   return contentPrefService->Set(NS_ConvertUTF8toUTF16(docUriSpec),
>                                  CPS_PREF_NAME, prefValue, loadContext,
>-                                 nullptr);
>+                                 nullptr, getter_AddRefs(pending));
> }
> 
> /**
>  * Forgets the current dictionary stored for aEditor's document URL.
>  */
> static nsresult
> ClearCurrentDictionary(nsIEditor* aEditor)
> {
>@@ -227,18 +229,20 @@ ClearCurrentDictionary(nsIEditor* aEdito
>   rv = docUri->GetSpec(docUriSpec);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   nsCOMPtr<nsIContentPrefService2> contentPrefService =
>     do_GetService(NS_CONTENT_PREF_SERVICE_CONTRACTID);
>   NS_ENSURE_TRUE(contentPrefService, NS_ERROR_NOT_INITIALIZED);
> 
>   nsCOMPtr<nsILoadContext> loadContext = GetLoadContext(aEditor);
>+  nsCOMPtr<nsIContentPrefPending> pending;
>   return contentPrefService->RemoveByDomainAndName(
>-    NS_ConvertUTF8toUTF16(docUriSpec), CPS_PREF_NAME, loadContext, nullptr);
>+    NS_ConvertUTF8toUTF16(docUriSpec), CPS_PREF_NAME, loadContext, nullptr,
>+    getter_AddRefs(pending));
> }
> 
> NS_IMPL_CYCLE_COLLECTING_ADDREF(nsEditorSpellCheck)
> NS_IMPL_CYCLE_COLLECTING_RELEASE(nsEditorSpellCheck)
> 
> NS_INTERFACE_MAP_BEGIN(nsEditorSpellCheck)
>   NS_INTERFACE_MAP_ENTRY(nsIEditorSpellCheck)
>   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIEditorSpellCheck)
>diff --git a/toolkit/components/contentprefs/ContentPrefService2.jsm b/toolkit/components/contentprefs/ContentPrefService2.jsm
>--- a/toolkit/components/contentprefs/ContentPrefService2.jsm
>+++ b/toolkit/components/contentprefs/ContentPrefService2.jsm
>@@ -38,28 +38,28 @@ function ContentPrefService2(cps) {
>   this._pbStore = cps._privModeStorage;
> }
> 
> ContentPrefService2.prototype = {
> 
>   getByDomainAndName: function CPS2_getByDomainAndName(group, name, context,
>                                                        callback) {
>     checkGroupArg(group);
>-    this._get(group, name, false, context, callback);
>+    return this._get(group, name, false, context, callback);
>   },
> 
>   getBySubdomainAndName: function CPS2_getBySubdomainAndName(group, name,
>                                                              context,
>                                                              callback) {
>     checkGroupArg(group);
>-    this._get(group, name, true, context, callback);
>+    return this._get(group, name, true, context, callback);
>   },
> 
>   getGlobal: function CPS2_getGlobal(name, context, callback) {
>-    this._get(null, name, false, context, callback);
>+    return this._get(null, name, false, context, callback);
>   },
> 
>   _get: function CPS2__get(group, name, includeSubdomains, context, callback) {
>     group = this._parseGroup(group);
>     checkNameArg(name);
>     checkCallbackArg(callback, true);
> 
>     // Some prefs may be in both the database and the private browsing store.
>@@ -68,17 +68,18 @@ ContentPrefService2.prototype = {
>     let pbPrefs = new ContentPrefStore();
>     if (context && context.usePrivateBrowsing) {
>       for (let [sgroup, val] in
>              this._pbStore.match(group, name, includeSubdomains)) {
>         pbPrefs.set(sgroup, name, val);
>       }
>     }
> 
>-    this._execStmts([this._commonGetStmt(group, name, includeSubdomains)], {
>+    let stmts = [this._commonGetStmt(group, name, includeSubdomains)];
>+    return this._execStmts(stmts, {
>       onRow: function onRow(row) {
>         let grp = row.getResultByName("grp");
>         let val = row.getResultByName("value");
>         this._cache.set(grp, name, val);
>         if (!pbPrefs.has(group, name))
>           cbHandleResult(callback, new ContentPref(grp, name, val));
>       },
>       onDone: function onDone(reason, ok, gotRow) {
>@@ -174,36 +175,42 @@ ContentPrefService2.prototype = {
>     for (let [sgroup, sname, val] in outStore) {
>       prefs.push(new ContentPref(sgroup, sname, val));
>     }
>     return prefs;
>   },
> 
>   set: function CPS2_set(group, name, value, context, callback) {
>     checkGroupArg(group);
>-    this._set(group, name, value, context, callback);
>+    return this._set(group, name, value, context, callback);
>   },
> 
>   setGlobal: function CPS2_setGlobal(name, value, context, callback) {
>-    this._set(null, name, value, context, callback);
>+    return this._set(null, name, value, context, callback);
>   },
> 
>   _set: function CPS2__set(group, name, value, context, callback) {
>     group = this._parseGroup(group);
>     checkNameArg(name);
>     checkValueArg(value);
>     checkCallbackArg(callback, false);
> 
>     if (context && context.usePrivateBrowsing) {
>-      this._pbStore.set(group, name, value);
>+      let pseudoPending = new PseudoPending();
>       this._schedule(function () {
>+        if (pseudoPending.canceled) {
>+          cbHandleCompletion(callback,
>+                             Ci.nsIContentPrefCallback2.COMPLETE_CANCELED);
>+          return;
>+        }
>+        this._pbStore.set(group, name, value);
>         cbHandleCompletion(callback, Ci.nsIContentPrefCallback2.COMPLETE_OK);
>         this._cps._broadcastPrefSet(group, name, value);
>       });
>-      return;
>+      return pseudoPending;
>     }
> 
>     // Invalidate the cached value so consumers accessing the cache between now
>     // and when the operation finishes don't get old data.
>     this._cache.remove(group, name);
> 
>     let stmts = [];
> 
>@@ -255,17 +262,17 @@ ContentPrefService2.prototype = {
>           ":value",
>         ")"
>       );
>     }
>     stmt.params.name = name;
>     stmt.params.value = value;
>     stmts.push(stmt);
> 
>-    this._execStmts(stmts, {
>+    return this._execStmts(stmts, {
>       onDone: function onDone(reason, ok) {
>         if (ok)
>           this._cache.setWithCast(group, name, value);
>         cbHandleCompletion(callback, reason);
>         if (ok)
>           this._cps._broadcastPrefSet(group, name, value);
>       },
>       onError: function onError(nsresult) {
>@@ -273,28 +280,28 @@ ContentPrefService2.prototype = {
>       }
>     });
>   },
> 
>   removeByDomainAndName: function CPS2_removeByDomainAndName(group, name,
>                                                              context,
>                                                              callback) {
>     checkGroupArg(group);
>-    this._remove(group, name, false, context, callback);
>+    return this._remove(group, name, false, context, callback);
>   },
> 
>   removeBySubdomainAndName: function CPS2_removeBySubdomainAndName(group, name,
>                                                                    context,
>                                                                    callback) {
>     checkGroupArg(group);
>-    this._remove(group, name, true, context, callback);
>+    return this._remove(group, name, true, context, callback);
>   },
> 
>   removeGlobal: function CPS2_removeGlobal(name, context,callback) {
>-    this._remove(null, name, false, context, callback);
>+    return this._remove(null, name, false, context, callback);
>   },
> 
>   _remove: function CPS2__remove(group, name, includeSubdomains, context,
>                                  callback) {
>     group = this._parseGroup(group);
>     checkNameArg(name);
>     checkCallbackArg(callback, false);
> 
>@@ -331,24 +338,26 @@ ContentPrefService2.prototype = {
>     stmts.push(this._stmt(
>       "DELETE FROM groups WHERE id NOT IN (",
>         "SELECT DISTINCT groupID FROM prefs WHERE groupID NOTNULL",
>       ")"
>     ));
> 
>     let prefs = new ContentPrefStore();
> 
>-    this._execStmts(stmts, {
>+    return this._execStmts(stmts, {
>       onRow: function onRow(row) {
>         let grp = row.getResultByName("grp");
>         prefs.set(grp, name, undefined);
>-        this._cache.set(grp, name, undefined);
>       },
>       onDone: function onDone(reason, ok) {
>         if (ok) {
>+          for (let [sgroup, , ] in prefs) {
>+            this._cache.set(sgroup, name, undefined);
>+          }
>           this._cache.set(group, name, undefined);
>           if (context && context.usePrivateBrowsing) {
>             for (let [sgroup, ] in
>                    this._pbStore.match(group, name, includeSubdomains)) {
>               prefs.set(sgroup, name, undefined);
>               this._pbStore.remove(sgroup, name);
>             }
>           }
>@@ -363,26 +372,26 @@ ContentPrefService2.prototype = {
>       onError: function onError(nsresult) {
>         cbHandleError(callback, nsresult);
>       }
>     });
>   },
> 
>   removeByDomain: function CPS2_removeByDomain(group, context, callback) {
>     checkGroupArg(group);
>-    this._removeByDomain(group, false, context, callback);
>+    return this._removeByDomain(group, false, context, callback);
>   },
> 
>   removeBySubdomain: function CPS2_removeBySubdomain(group, context, callback) {
>     checkGroupArg(group);
>-    this._removeByDomain(group, true, context, callback);
>+    return this._removeByDomain(group, true, context, callback);
>   },
> 
>   removeAllGlobals: function CPS2_removeAllGlobals(context, callback) {
>-    this._removeByDomain(null, false, context, callback);
>+    return this._removeByDomain(null, false, context, callback);
>   },
> 
>   _removeByDomain: function CPS2__removeByDomain(group, includeSubdomains,
>                                                  context, callback) {
>     group = this._parseGroup(group);
>     checkCallbackArg(callback, false);
> 
>     // Invalidate the cached values so consumers accessing the cache between now
>@@ -426,28 +435,32 @@ ContentPrefService2.prototype = {
>     // Finally delete settings that are no longer referenced.
>     stmts.push(this._stmt(
>       "DELETE FROM settings",
>       "WHERE id NOT IN (SELECT DISTINCT settingID FROM prefs)"
>     ));
> 
>     let prefs = new ContentPrefStore();
> 
>-    this._execStmts(stmts, {
>+    return this._execStmts(stmts, {
>       onRow: function onRow(row) {
>         let grp = row.getResultByName("grp");
>         let name = row.getResultByName("name");
>         prefs.set(grp, name, undefined);
>-        this._cache.set(grp, name, undefined);
>       },
>       onDone: function onDone(reason, ok) {
>-        if (ok && context && context.usePrivateBrowsing) {
>-          for (let [sgroup, sname, ] in this._pbStore) {
>-            prefs.set(sgroup, sname, undefined);
>-            this._pbStore.remove(sgroup, sname);
>+        if (ok) {
>+          for (let [sgroup, sname, ] in prefs) {
>+            this._cache.set(sgroup, sname, undefined);
>+          }
>+          if (context && context.usePrivateBrowsing) {
>+            for (let [sgroup, sname, ] in this._pbStore) {
>+              prefs.set(sgroup, sname, undefined);
>+              this._pbStore.remove(sgroup, sname);
>+            }
>           }
>         }
>         cbHandleCompletion(callback, reason);
>         if (ok) {
>           for (let [sgroup, sname, ] in prefs) {
>             this._cps._broadcastPrefRemoved(sgroup, sname);
>           }
>         }
>@@ -483,29 +496,33 @@ ContentPrefService2.prototype = {
>     ));
>     stmts.push(this._stmt(
>       "DELETE FROM settings",
>       "WHERE id NOT IN (SELECT DISTINCT settingID FROM prefs)"
>     ));
> 
>     let prefs = new ContentPrefStore();
> 
>-    this._execStmts(stmts, {
>+    return this._execStmts(stmts, {
>       onRow: function onRow(row) {
>         let grp = row.getResultByName("grp");
>         let name = row.getResultByName("name");
>         prefs.set(grp, name, undefined);
>-        this._cache.set(grp, name, undefined);
>       },
>       onDone: function onDone(reason, ok) {
>-        if (ok && context && context.usePrivateBrowsing) {
>-          for (let [sgroup, sname, ] in this._pbStore) {
>-            prefs.set(sgroup, sname, undefined);
>+        if (ok) {
>+          for (let [sgroup, sname, ] in prefs) {
>+            this._cache.set(sgroup, sname, undefined);
>           }
>-          this._pbStore.removeAllGroups();
>+          if (context && context.usePrivateBrowsing) {
>+            for (let [sgroup, sname, ] in this._pbStore) {
>+              prefs.set(sgroup, sname, undefined);
>+            }
>+            this._pbStore.removeAllGroups();
>+          }
>         }
>         cbHandleCompletion(callback, reason);
>         if (ok) {
>           for (let [sgroup, sname, ] in prefs) {
>             this._cps._broadcastPrefRemoved(sgroup, sname);
>           }
>         }
>       },
>@@ -562,28 +579,32 @@ ContentPrefService2.prototype = {
>     stmts.push(this._stmt(
>       "DELETE FROM groups WHERE id NOT IN (",
>         "SELECT DISTINCT groupID FROM prefs WHERE groupID NOTNULL",
>       ")"
>     ));
> 
>     let prefs = new ContentPrefStore();
> 
>-    this._execStmts(stmts, {
>+    return this._execStmts(stmts, {
>       onRow: function onRow(row) {
>         let grp = row.getResultByName("grp");
>         prefs.set(grp, name, undefined);
>-        this._cache.set(grp, name, undefined);
>       },
>       onDone: function onDone(reason, ok) {
>-        if (ok && context && context.usePrivateBrowsing) {
>-          for (let [sgroup, sname, ] in this._pbStore) {
>-            if (sname === name) {
>-              prefs.set(sgroup, name, undefined);
>-              this._pbStore.remove(sgroup, name);
>+        if (ok) {
>+          for (let [sgroup, , ] in prefs) {
>+            this._cache.set(sgroup, name, undefined);
>+          }
>+          if (context && context.usePrivateBrowsing) {
>+            for (let [sgroup, sname, ] in this._pbStore) {
>+              if (sname === name) {
>+                prefs.set(sgroup, name, undefined);
>+                this._pbStore.remove(sgroup, name);
>+              }
>             }
>           }
>         }
>         cbHandleCompletion(callback, reason);
>         if (ok) {
>           for (let [sgroup, , ] in prefs) {
>             this._cps._broadcastPrefRemoved(sgroup, name);
>           }
>@@ -631,56 +652,61 @@ ContentPrefService2.prototype = {
>    *                   onDone(reason, reasonOK, didGetRow) (required)
>    *                     Called when done.
>    *                     reason: A nsIContentPrefService2.COMPLETE_* value.
>    *                     reasonOK: reason == nsIContentPrefService2.COMPLETE_OK.
>    *                     didGetRow: True if onRow was ever called.
>    *                   onError(nsresult) (optional)
>    *                     Called on error.
>    *                     nsresult: The error code.
>+   * @return StoragePending
>    */
>   _execStmts: function CPS2__execStmts(stmts, callbacks) {
>     let self = this;
>     let gotRow = false;
>-    this._cps._dbConnection.executeAsync(stmts, stmts.length, {
>+    let pending = this._cps._dbConnection.executeAsync(stmts, stmts.length, {
>       handleResult: function handleResult(results) {
>         try {
>           let row = null;
>           while ((row = results.getNextRow())) {
>             gotRow = true;
>             if (callbacks.onRow)
>               callbacks.onRow.call(self, row);
>           }
>         }
>         catch (err) {
>           Cu.reportError(err);
>         }
>       },
>       handleCompletion: function handleCompletion(reason) {
>         try {
>-          let ok = reason == Ci.mozIStorageStatementCallback.REASON_FINISHED;
>-          callbacks.onDone.call(self,
>-                                ok ? Ci.nsIContentPrefCallback2.COMPLETE_OK :
>-                                  Ci.nsIContentPrefCallback2.COMPLETE_ERROR,
>-                                ok, gotRow);
>+          let cpsReason =
>+            reason == Ci.mozIStorageStatementCallback.REASON_FINISHED ?
>+              Ci.nsIContentPrefCallback2.COMPLETE_OK :
>+            reason == Ci.mozIStorageStatementCallback.REASON_CANCELED ?
>+              Ci.nsIContentPrefCallback2.COMPLETE_CANCELED :
>+            Ci.nsIContentPrefCallback2.COMPLETE_ERROR;
>+          let ok = cpsReason == Ci.nsIContentPrefCallback2.COMPLETE_OK;
>+          callbacks.onDone.call(self, cpsReason, ok, gotRow);
>         }
>         catch (err) {
>           Cu.reportError(err);
>         }
>       },
>       handleError: function handleError(error) {
>         try {
>           if (callbacks.onError)
>             callbacks.onError.call(self, Cr.NS_ERROR_FAILURE);
>         }
>         catch (err) {
>           Cu.reportError(err);
>         }
>       }
>     });
>+    return new StoragePending(pending);
>   },
> 
>   /**
>    * Parses the domain (the "group", to use the database's term) from the given
>    * string.
>    *
>    * @param groupStr  Assumed to be either a string or falsey.
>    * @return          If groupStr is a valid URL string, returns the domain of
>@@ -825,8 +851,30 @@ function checkCallbackArg(callback, requ
> 
> function invalidArg(msg) {
>   return Components.Exception(msg, Cr.NS_ERROR_INVALID_ARG);
> }
> 
> function joinArgs(args) {
>   return Array.join(args, " ").trim().replace(/\s{2,}/g, " ");
> }
>+
>+function StoragePending(pending) {
>+  this._pending = pending;
>+}
>+
>+StoragePending.prototype = {
>+  cancel: function StoragePending_cancel() {
>+    this._pending.cancel();
>+  },
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIContentPrefPending]),
>+};
>+
>+function PseudoPending() {
>+  this.canceled = false;
>+}
>+
>+PseudoPending.prototype = {
>+  cancel: function StoragePending_cancel() {
>+    this.canceled = true;
>+  },
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIContentPrefPending]),
>+};
>diff --git a/toolkit/components/contentprefs/tests/unit_cps2/head.js b/toolkit/components/contentprefs/tests/unit_cps2/head.js
>--- a/toolkit/components/contentprefs/tests/unit_cps2/head.js
>+++ b/toolkit/components/contentprefs/tests/unit_cps2/head.js
>@@ -153,17 +153,18 @@ function getGlobalOK(args, expectedVal) 
>   yield getOKEx("getGlobal", args, expectedPrefs);
> }
> 
> function getOKEx(methodName, args, expectedPrefs, strict, context) {
>   let actualPrefs = [];
>   args.push(makeCallback({
>     handleResult: function (pref) actualPrefs.push(pref)
>   }));
>-  yield cps[methodName].apply(cps, args);
>+  cps[methodName].apply(cps, args);
>+  yield;
>   arraysOfArraysOK([actualPrefs], [expectedPrefs], function (actual, expected) {
>     prefOK(actual, expected, strict);
>   });
> }
> 
> function getCachedOK(args, expectedIsCached, expectedVal, expectedGroup,
>                      strict) {
>   if (args.length == 2)
>diff --git a/toolkit/components/contentprefs/tests/unit_cps2/test_getCachedSubdomains.js b/toolkit/components/contentprefs/tests/unit_cps2/test_getCachedSubdomains.js
>--- a/toolkit/components/contentprefs/tests/unit_cps2/test_getCachedSubdomains.js
>+++ b/toolkit/components/contentprefs/tests/unit_cps2/test_getCachedSubdomains.js
>@@ -42,116 +42,131 @@ let tests = [
>   function subdomains() {
>     yield set("a.com", "foo", 1);
>     yield set("b.a.com", "foo", 2);
>     getCachedSubdomainsOK(["a.com", "foo"], [["a.com", 1], ["b.a.com", 2]]);
>     getCachedSubdomainsOK(["b.a.com", "foo"], [["b.a.com", 2]]);
>   },
> 
>   function populateViaGet() {
>-    yield cps.getByDomainAndName("a.com", "foo", null, makeCallback());
>+    cps.getByDomainAndName("a.com", "foo", null, makeCallback());
>+    yield;
>     getCachedSubdomainsOK(["a.com", "foo"], [["a.com", undefined]]);
> 
>-    yield cps.getGlobal("foo", null, makeCallback());
>+    cps.getGlobal("foo", null, makeCallback());
>+    yield;
>     getCachedSubdomainsOK(["a.com", "foo"], [["a.com", undefined]]);
>     getCachedGlobalOK(["foo"], true, undefined);
>   },
> 
>   function populateViaGetSubdomains() {
>-    yield cps.getBySubdomainAndName("a.com", "foo", null, makeCallback());
>+    cps.getBySubdomainAndName("a.com", "foo", null, makeCallback());
>+    yield;
>     getCachedSubdomainsOK(["a.com", "foo"], [["a.com", undefined]]);
>   },
> 
>   function populateViaRemove() {
>-    yield cps.removeByDomainAndName("a.com", "foo", null, makeCallback());
>+    cps.removeByDomainAndName("a.com", "foo", null, makeCallback());
>+    yield;
>     getCachedSubdomainsOK(["a.com", "foo"], [["a.com", undefined]]);
> 
>-    yield cps.removeBySubdomainAndName("b.com", "foo", null, makeCallback());
>+    cps.removeBySubdomainAndName("b.com", "foo", null, makeCallback());
>+    yield;
>     getCachedSubdomainsOK(["a.com", "foo"], [["a.com", undefined]]);
>     getCachedSubdomainsOK(["b.com", "foo"], [["b.com", undefined]]);
> 
>-    yield cps.removeGlobal("foo", null, makeCallback());
>+    cps.removeGlobal("foo", null, makeCallback());
>+    yield;
>     getCachedSubdomainsOK(["a.com", "foo"], [["a.com", undefined]]);
>     getCachedSubdomainsOK(["b.com", "foo"], [["b.com", undefined]]);
>     getCachedGlobalOK(["foo"], true, undefined);
> 
>     yield set("a.com", "foo", 1);
>-    yield cps.removeByDomainAndName("a.com", "foo", null, makeCallback());
>+    cps.removeByDomainAndName("a.com", "foo", null, makeCallback());
>+    yield;
>     getCachedSubdomainsOK(["a.com", "foo"], [["a.com", undefined]]);
>     getCachedSubdomainsOK(["b.com", "foo"], [["b.com", undefined]]);
>     getCachedGlobalOK(["foo"], true, undefined);
> 
>     yield set("a.com", "foo", 2);
>     yield set("b.a.com", "foo", 3);
>-    yield cps.removeBySubdomainAndName("a.com", "foo", null, makeCallback());
>+    cps.removeBySubdomainAndName("a.com", "foo", null, makeCallback());
>+    yield;
>     getCachedSubdomainsOK(["a.com", "foo"],
>                           [["a.com", undefined], ["b.a.com", undefined]]);
>     getCachedSubdomainsOK(["b.com", "foo"], [["b.com", undefined]]);
>     getCachedGlobalOK(["foo"], true, undefined);
>     getCachedSubdomainsOK(["b.a.com", "foo"], [["b.a.com", undefined]]);
> 
>     yield setGlobal("foo", 4);
>-    yield cps.removeGlobal("foo", null, makeCallback());
>+    cps.removeGlobal("foo", null, makeCallback());
>+    yield;
>     getCachedSubdomainsOK(["a.com", "foo"],
>                           [["a.com", undefined], ["b.a.com", undefined]]);
>     getCachedSubdomainsOK(["b.com", "foo"], [["b.com", undefined]]);
>     getCachedGlobalOK(["foo"], true, undefined);
>     getCachedSubdomainsOK(["b.a.com", "foo"], [["b.a.com", undefined]]);
>   },
> 
>   function populateViaRemoveByDomain() {
>     yield set("a.com", "foo", 1);
>     yield set("a.com", "bar", 2);
>     yield set("b.a.com", "foo", 3);
>     yield set("b.a.com", "bar", 4);
>-    yield cps.removeByDomain("a.com", null, makeCallback());
>+    cps.removeByDomain("a.com", null, makeCallback());
>+    yield;
>     getCachedSubdomainsOK(["a.com", "foo"],
>                           [["a.com", undefined], ["b.a.com", 3]]);
>     getCachedSubdomainsOK(["a.com", "bar"],
>                           [["a.com", undefined], ["b.a.com", 4]]);
> 
>     yield set("a.com", "foo", 5);
>     yield set("a.com", "bar", 6);
>-    yield cps.removeBySubdomain("a.com", null, makeCallback());
>+    cps.removeBySubdomain("a.com", null, makeCallback());
>+    yield;
>     getCachedSubdomainsOK(["a.com", "foo"],
>                           [["a.com", undefined], ["b.a.com", undefined]]);
>     getCachedSubdomainsOK(["a.com", "bar"],
>                           [["a.com", undefined], ["b.a.com", undefined]]);
> 
>     yield setGlobal("foo", 7);
>     yield setGlobal("bar", 8);
>-    yield cps.removeAllGlobals(null, makeCallback());
>+    cps.removeAllGlobals(null, makeCallback());
>+    yield;
>     getCachedGlobalOK(["foo"], true, undefined);
>     getCachedGlobalOK(["bar"], true, undefined);
>   },
> 
>   function populateViaRemoveAllDomains() {
>     yield set("a.com", "foo", 1);
>     yield set("a.com", "bar", 2);
>     yield set("b.com", "foo", 3);
>     yield set("b.com", "bar", 4);
>-    yield cps.removeAllDomains(null, makeCallback());
>+    cps.removeAllDomains(null, makeCallback());
>+    yield;
>     getCachedSubdomainsOK(["a.com", "foo"], [["a.com", undefined]]);
>     getCachedSubdomainsOK(["a.com", "bar"], [["a.com", undefined]]);
>     getCachedSubdomainsOK(["b.com", "foo"], [["b.com", undefined]]);
>     getCachedSubdomainsOK(["b.com", "bar"], [["b.com", undefined]]);
>   },
> 
>   function populateViaRemoveByName() {
>     yield set("a.com", "foo", 1);
>     yield set("a.com", "bar", 2);
>     yield setGlobal("foo", 3);
>     yield setGlobal("bar", 4);
>-    yield cps.removeByName("foo", null, makeCallback());
>+    cps.removeByName("foo", null, makeCallback());
>+    yield;
>     getCachedSubdomainsOK(["a.com", "foo"], [["a.com", undefined]]);
>     getCachedSubdomainsOK(["a.com", "bar"], [["a.com", 2]]);
>     getCachedGlobalOK(["foo"], true, undefined);
>     getCachedGlobalOK(["bar"], true, 4);
> 
>-    yield cps.removeByName("bar", null, makeCallback());
>+    cps.removeByName("bar", null, makeCallback());
>+    yield;
>     getCachedSubdomainsOK(["a.com", "foo"], [["a.com", undefined]]);
>     getCachedSubdomainsOK(["a.com", "bar"], [["a.com", undefined]]);
>     getCachedGlobalOK(["foo"], true, undefined);
>     getCachedGlobalOK(["bar"], true, undefined);
>   },
> 
>   function privateBrowsing() {
>     yield set("a.com", "foo", 1);
>diff --git a/toolkit/components/contentprefs/tests/unit_cps2/test_observers.js b/toolkit/components/contentprefs/tests/unit_cps2/test_observers.js
>--- a/toolkit/components/contentprefs/tests/unit_cps2/test_observers.js
>+++ b/toolkit/components/contentprefs/tests/unit_cps2/test_observers.js
>@@ -21,83 +21,92 @@ let tests = [
>     observerArgsOK(args.null, [[null, "foo", 2]]);
>     observerArgsOK(args.bar, []);
>   },
> 
>   function observerForName_remove() {
>     yield set("a.com", "foo", 1);
>     yield setGlobal("foo", 2);
> 
>-    yield cps.removeByDomainAndName("a.com", "bogus", null, makeCallback());
>+    cps.removeByDomainAndName("a.com", "bogus", null, makeCallback());
>+    yield;
>     let args = yield on("Removed", ["foo", null, "bar"]);
>     observerArgsOK(args.foo, []);
>     observerArgsOK(args.null, []);
>     observerArgsOK(args.bar, []);
> 
>-    yield cps.removeByDomainAndName("a.com", "foo", null, makeCallback());
>+    cps.removeByDomainAndName("a.com", "foo", null, makeCallback());
>+    yield;
>     args = yield on("Removed", ["foo", null, "bar"]);
>     observerArgsOK(args.foo, [["a.com", "foo"]]);
>     observerArgsOK(args.null, [["a.com", "foo"]]);
>     observerArgsOK(args.bar, []);
> 
>-    yield cps.removeGlobal("foo", null, makeCallback());
>+    cps.removeGlobal("foo", null, makeCallback());
>+    yield;
>     args = yield on("Removed", ["foo", null, "bar"]);
>     observerArgsOK(args.foo, [[null, "foo"]]);
>     observerArgsOK(args.null, [[null, "foo"]]);
>     observerArgsOK(args.bar, []);
>   },
> 
>   function observerForName_removeByDomain() {
>     yield set("a.com", "foo", 1);
>     yield set("b.a.com", "bar", 2);
>     yield setGlobal("foo", 3);
> 
>-    yield cps.removeByDomain("bogus", null, makeCallback());
>+    cps.removeByDomain("bogus", null, makeCallback());
>+    yield;
>     let args = yield on("Removed", ["foo", null, "bar"]);
>     observerArgsOK(args.foo, []);
>     observerArgsOK(args.null, []);
>     observerArgsOK(args.bar, []);
> 
>-    yield cps.removeBySubdomain("a.com", null, makeCallback());
>+    cps.removeBySubdomain("a.com", null, makeCallback());
>+    yield;
>     args = yield on("Removed", ["foo", null, "bar"]);
>     observerArgsOK(args.foo, [["a.com", "foo"]]);
>     observerArgsOK(args.null, [["a.com", "foo"], ["b.a.com", "bar"]]);
>     observerArgsOK(args.bar, [["b.a.com", "bar"]]);
> 
>-    yield cps.removeAllGlobals(null, makeCallback());
>+    cps.removeAllGlobals(null, makeCallback());
>+    yield;
>     args = yield on("Removed", ["foo", null, "bar"]);
>     observerArgsOK(args.foo, [[null, "foo"]]);
>     observerArgsOK(args.null, [[null, "foo"]]);
>     observerArgsOK(args.bar, []);
>   },
> 
>   function observerForName_removeAllDomains() {
>     yield set("a.com", "foo", 1);
>     yield setGlobal("foo", 2);
>     yield set("b.com", "bar", 3);
> 
>-    yield cps.removeAllDomains(null, makeCallback());
>+    cps.removeAllDomains(null, makeCallback());
>+    yield;
>     let args = yield on("Removed", ["foo", null, "bar"]);
>     observerArgsOK(args.foo, [["a.com", "foo"]]);
>     observerArgsOK(args.null, [["a.com", "foo"], ["b.com", "bar"]]);
>     observerArgsOK(args.bar, [["b.com", "bar"]]);
>   },
> 
>   function observerForName_removeByName() {
>     yield set("a.com", "foo", 1);
>     yield set("a.com", "bar", 2);
>     yield setGlobal("foo", 3);
> 
>-    yield cps.removeByName("bogus", null, makeCallback());
>+    cps.removeByName("bogus", null, makeCallback());
>+    yield;
>     let args = yield on("Removed", ["foo", null, "bar"]);
>     observerArgsOK(args.foo, []);
>     observerArgsOK(args.null, []);
>     observerArgsOK(args.bar, []);
> 
>-    yield cps.removeByName("foo", null, makeCallback());
>+    cps.removeByName("foo", null, makeCallback());
>+    yield;
>     args = yield on("Removed", ["foo", null, "bar"]);
>     observerArgsOK(args.foo, [["a.com", "foo"], [null, "foo"]]);
>     observerArgsOK(args.null, [["a.com", "foo"], [null, "foo"]]);
>     observerArgsOK(args.bar, []);
>   },
> 
>   function removeObserverForName() {
>     let args = yield on("Set", ["foo", null, "bar"], true);
>diff --git a/toolkit/components/contentprefs/tests/unit_cps2/test_remove.js b/toolkit/components/contentprefs/tests/unit_cps2/test_remove.js
>--- a/toolkit/components/contentprefs/tests/unit_cps2/test_remove.js
>+++ b/toolkit/components/contentprefs/tests/unit_cps2/test_remove.js
>@@ -7,135 +7,148 @@ function run_test() {
> }
> 
> let tests = [
> 
>   function nonexistent() {
>     yield set("a.com", "foo", 1);
>     yield setGlobal("foo", 2);
> 
>-    yield cps.removeByDomainAndName("a.com", "bogus", null, makeCallback());
>+    cps.removeByDomainAndName("a.com", "bogus", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "foo", 1],
>       [null, "foo", 2],
>     ]);
>     yield getOK(["a.com", "foo"], 1);
>     yield getGlobalOK(["foo"], 2);
> 
>-    yield cps.removeBySubdomainAndName("a.com", "bogus", null, makeCallback());
>+    cps.removeBySubdomainAndName("a.com", "bogus", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "foo", 1],
>       [null, "foo", 2],
>     ]);
>     yield getOK(["a.com", "foo"], 1);
>     yield getGlobalOK(["foo"], 2);
> 
>-    yield cps.removeGlobal("bogus", null, makeCallback());
>+    cps.removeGlobal("bogus", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "foo", 1],
>       [null, "foo", 2],
>     ]);
>     yield getOK(["a.com", "foo"], 1);
>     yield getGlobalOK(["foo"], 2);
> 
>-    yield cps.removeByDomainAndName("bogus", "bogus", null, makeCallback());
>+    cps.removeByDomainAndName("bogus", "bogus", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "foo", 1],
>       [null, "foo", 2],
>     ]);
>     yield getOK(["a.com", "foo"], 1);
>     yield getGlobalOK(["foo"], 2);
>   },
> 
>   function isomorphicDomains() {
>     yield set("a.com", "foo", 1);
>-    yield cps.removeByDomainAndName("a.com", "foo", null, makeCallback());
>+    cps.removeByDomainAndName("a.com", "foo", null, makeCallback());
>+    yield;
>     yield dbOK([]);
>     yield getOK(["a.com", "foo"], undefined);
> 
>     yield set("a.com", "foo", 2);
>-    yield cps.removeByDomainAndName("http://a.com/huh", "foo", null,
>-                                    makeCallback());
>+    cps.removeByDomainAndName("http://a.com/huh", "foo", null, makeCallback());
>+    yield;
>     yield dbOK([]);
>     yield getOK(["a.com", "foo"], undefined);
>   },
> 
>   function names() {
>     yield set("a.com", "foo", 1);
>     yield set("a.com", "bar", 2);
>     yield setGlobal("foo", 3);
>     yield setGlobal("bar", 4);
> 
>-    yield cps.removeByDomainAndName("a.com", "foo", null, makeCallback());
>+    cps.removeByDomainAndName("a.com", "foo", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "bar", 2],
>       [null, "foo", 3],
>       [null, "bar", 4],
>     ]);
>     yield getOK(["a.com", "foo"], undefined);
>     yield getOK(["a.com", "bar"], 2);
>     yield getGlobalOK(["foo"], 3);
>     yield getGlobalOK(["bar"], 4);
> 
>-    yield cps.removeGlobal("foo", null, makeCallback());
>+    cps.removeGlobal("foo", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "bar", 2],
>       [null, "bar", 4],
>     ]);
>     yield getOK(["a.com", "foo"], undefined);
>     yield getOK(["a.com", "bar"], 2);
>     yield getGlobalOK(["foo"], undefined);
>     yield getGlobalOK(["bar"], 4);
> 
>-    yield cps.removeByDomainAndName("a.com", "bar", null, makeCallback());
>+    cps.removeByDomainAndName("a.com", "bar", null, makeCallback());
>+    yield;
>     yield dbOK([
>       [null, "bar", 4],
>     ]);
>     yield getOK(["a.com", "foo"], undefined);
>     yield getOK(["a.com", "bar"], undefined);
>     yield getGlobalOK(["foo"], undefined);
>     yield getGlobalOK(["bar"], 4);
> 
>-    yield cps.removeGlobal("bar", null, makeCallback());
>+    cps.removeGlobal("bar", null, makeCallback());
>+    yield;
>     yield dbOK([
>     ]);
>     yield getOK(["a.com", "foo"], undefined);
>     yield getOK(["a.com", "bar"], undefined);
>     yield getGlobalOK(["foo"], undefined);
>     yield getGlobalOK(["bar"], undefined);
>   },
> 
>   function subdomains() {
>     yield set("a.com", "foo", 1);
>     yield set("b.a.com", "foo", 2);
>-    yield cps.removeByDomainAndName("a.com", "foo", null, makeCallback());
>+    cps.removeByDomainAndName("a.com", "foo", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["b.a.com", "foo", 2],
>     ]);
>     yield getSubdomainsOK(["a.com", "foo"], [["b.a.com", 2]]);
>     yield getSubdomainsOK(["b.a.com", "foo"], [["b.a.com", 2]]);
> 
>     yield set("a.com", "foo", 3);
>-    yield cps.removeBySubdomainAndName("a.com", "foo", null, makeCallback());
>+    cps.removeBySubdomainAndName("a.com", "foo", null, makeCallback());
>+    yield;
>     yield dbOK([
>     ]);
>     yield getSubdomainsOK(["a.com", "foo"], []);
>     yield getSubdomainsOK(["b.a.com", "foo"], []);
> 
>     yield set("a.com", "foo", 4);
>     yield set("b.a.com", "foo", 5);
>-    yield cps.removeByDomainAndName("b.a.com", "foo", null, makeCallback());
>+    cps.removeByDomainAndName("b.a.com", "foo", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "foo", 4],
>     ]);
>     yield getSubdomainsOK(["a.com", "foo"], [["a.com", 4]]);
>     yield getSubdomainsOK(["b.a.com", "foo"], []);
> 
>     yield set("b.a.com", "foo", 6);
>-    yield cps.removeBySubdomainAndName("b.a.com", "foo", null, makeCallback());
>+    cps.removeBySubdomainAndName("b.a.com", "foo", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "foo", 4],
>     ]);
>     yield getSubdomainsOK(["a.com", "foo"], [["a.com", 4]]);
>     yield getSubdomainsOK(["b.a.com", "foo"], []);
>   },
> 
>   function privateBrowsing() {
>@@ -145,20 +158,24 @@ let tests = [
>     yield setGlobal("bar", 4);
>     yield setGlobal("qux", 5);
>     yield set("b.com", "foo", 6);
>     yield set("b.com", "bar", 7);
> 
>     let context = { usePrivateBrowsing: true };
>     yield set("a.com", "foo", 8, context);
>     yield setGlobal("foo", 9, context);
>-    yield cps.removeByDomainAndName("a.com", "foo", context, makeCallback());
>-    yield cps.removeGlobal("foo", context, makeCallback());
>-    yield cps.removeGlobal("qux", context, makeCallback());
>-    yield cps.removeByDomainAndName("b.com", "foo", context, makeCallback());
>+    cps.removeByDomainAndName("a.com", "foo", context, makeCallback());
>+    yield;
>+    cps.removeGlobal("foo", context, makeCallback());
>+    yield;
>+    cps.removeGlobal("qux", context, makeCallback());
>+    yield;
>+    cps.removeByDomainAndName("b.com", "foo", context, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "bar", 2],
>       [null, "bar", 4],
>       ["b.com", "bar", 7],
>     ]);
>     yield getOK(["a.com", "foo", context], undefined);
>     yield getOK(["a.com", "bar", context], 2);
>     yield getGlobalOK(["foo", context], undefined);
>diff --git a/toolkit/components/contentprefs/tests/unit_cps2/test_removeAllDomains.js b/toolkit/components/contentprefs/tests/unit_cps2/test_removeAllDomains.js
>--- a/toolkit/components/contentprefs/tests/unit_cps2/test_removeAllDomains.js
>+++ b/toolkit/components/contentprefs/tests/unit_cps2/test_removeAllDomains.js
>@@ -5,32 +5,34 @@
> function run_test() {
>   runAsyncTests(tests);
> }
> 
> let tests = [
> 
>   function nonexistent() {
>     yield setGlobal("foo", 1);
>-    yield cps.removeAllDomains(null, makeCallback());
>+    cps.removeAllDomains(null, makeCallback());
>+    yield;
>     yield dbOK([
>       [null, "foo", 1],
>     ]);
>     yield getGlobalOK(["foo"], 1);
>   },
> 
>   function domains() {
>     yield set("a.com", "foo", 1);
>     yield set("a.com", "bar", 2);
>     yield setGlobal("foo", 3);
>     yield setGlobal("bar", 4);
>     yield set("b.com", "foo", 5);
>     yield set("b.com", "bar", 6);
> 
>-    yield cps.removeAllDomains(null, makeCallback());
>+    cps.removeAllDomains(null, makeCallback());
>+    yield;
>     yield dbOK([
>       [null, "foo", 3],
>       [null, "bar", 4],
>     ]);
>     yield getOK(["a.com", "foo"], undefined);
>     yield getOK(["a.com", "bar"], undefined);
>     yield getGlobalOK(["foo"], 3);
>     yield getGlobalOK(["bar"], 4);
>@@ -43,17 +45,18 @@ let tests = [
>     yield set("a.com", "bar", 2);
>     yield setGlobal("foo", 3);
>     yield setGlobal("bar", 4);
>     yield set("b.com", "foo", 5);
> 
>     let context = { usePrivateBrowsing: true };
>     yield set("a.com", "foo", 6, context);
>     yield setGlobal("foo", 7, context);
>-    yield cps.removeAllDomains(context, makeCallback());
>+    cps.removeAllDomains(context, makeCallback());
>+    yield;
>     yield dbOK([
>       [null, "foo", 3],
>       [null, "bar", 4],
>     ]);
>     yield getOK(["a.com", "foo", context], undefined);
>     yield getOK(["a.com", "bar", context], undefined);
>     yield getGlobalOK(["foo", context], 7);
>     yield getGlobalOK(["bar", context], 4);
>diff --git a/toolkit/components/contentprefs/tests/unit_cps2/test_removeByDomain.js b/toolkit/components/contentprefs/tests/unit_cps2/test_removeByDomain.js
>--- a/toolkit/components/contentprefs/tests/unit_cps2/test_removeByDomain.js
>+++ b/toolkit/components/contentprefs/tests/unit_cps2/test_removeByDomain.js
>@@ -7,118 +7,129 @@ function run_test() {
> }
> 
> let tests = [
> 
>   function nonexistent() {
>     yield set("a.com", "foo", 1);
>     yield setGlobal("foo", 2);
> 
>-    yield cps.removeByDomain("bogus", null, makeCallback());
>+    cps.removeByDomain("bogus", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "foo", 1],
>       [null, "foo", 2],
>     ]);
>     yield getOK(["a.com", "foo"], 1);
>     yield getGlobalOK(["foo"], 2);
> 
>-    yield cps.removeBySubdomain("bogus", null, makeCallback());
>+    cps.removeBySubdomain("bogus", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "foo", 1],
>       [null, "foo", 2],
>     ]);
>     yield getOK(["a.com", "foo"], 1);
>     yield getGlobalOK(["foo"], 2);
>   },
> 
>   function isomorphicDomains() {
>     yield set("a.com", "foo", 1);
>-    yield cps.removeByDomain("a.com", null, makeCallback());
>+    cps.removeByDomain("a.com", null, makeCallback());
>+    yield;
>     yield dbOK([]);
>     yield getOK(["a.com", "foo"], undefined);
> 
>     yield set("a.com", "foo", 2);
>-    yield cps.removeByDomain("http://a.com/huh", null, makeCallback());
>+    cps.removeByDomain("http://a.com/huh", null, makeCallback());
>+    yield;
>     yield dbOK([]);
>     yield getOK(["a.com", "foo"], undefined);
>   },
> 
>   function domains() {
>     yield set("a.com", "foo", 1);
>     yield set("a.com", "bar", 2);
>     yield setGlobal("foo", 3);
>     yield setGlobal("bar", 4);
>     yield set("b.com", "foo", 5);
>     yield set("b.com", "bar", 6);
> 
>-    yield cps.removeByDomain("a.com", null, makeCallback());
>+    cps.removeByDomain("a.com", null, makeCallback());
>+    yield;
>     yield dbOK([
>       [null, "foo", 3],
>       [null, "bar", 4],
>       ["b.com", "foo", 5],
>       ["b.com", "bar", 6],
>     ]);
>     yield getOK(["a.com", "foo"], undefined);
>     yield getOK(["a.com", "bar"], undefined);
>     yield getGlobalOK(["foo"], 3);
>     yield getGlobalOK(["bar"], 4);
>     yield getOK(["b.com", "foo"], 5);
>     yield getOK(["b.com", "bar"], 6);
> 
>-    yield cps.removeAllGlobals(null, makeCallback());
>+    cps.removeAllGlobals(null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["b.com", "foo", 5],
>       ["b.com", "bar", 6],
>     ]);
>     yield getOK(["a.com", "foo"], undefined);
>     yield getOK(["a.com", "bar"], undefined);
>     yield getGlobalOK(["foo"], undefined);
>     yield getGlobalOK(["bar"], undefined);
>     yield getOK(["b.com", "foo"], 5);
>     yield getOK(["b.com", "bar"], 6);
> 
>-    yield cps.removeByDomain("b.com", null, makeCallback());
>+    cps.removeByDomain("b.com", null, makeCallback());
>+    yield;
>     yield dbOK([
>     ]);
>     yield getOK(["a.com", "foo"], undefined);
>     yield getOK(["a.com", "bar"], undefined);
>     yield getGlobalOK(["foo"], undefined);
>     yield getGlobalOK(["bar"], undefined);
>     yield getOK(["b.com", "foo"], undefined);
>     yield getOK(["b.com", "bar"], undefined);
>   },
> 
>   function subdomains() {
>     yield set("a.com", "foo", 1);
>     yield set("b.a.com", "foo", 2);
>-    yield cps.removeByDomain("a.com", null, makeCallback());
>+    cps.removeByDomain("a.com", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["b.a.com", "foo", 2],
>     ]);
>     yield getSubdomainsOK(["a.com", "foo"], [["b.a.com", 2]]);
>     yield getSubdomainsOK(["b.a.com", "foo"], [["b.a.com", 2]]);
> 
>     yield set("a.com", "foo", 3);
>-    yield cps.removeBySubdomain("a.com", null, makeCallback());
>+    cps.removeBySubdomain("a.com", null, makeCallback());
>+    yield;
>     yield dbOK([
>     ]);
>     yield getSubdomainsOK(["a.com", "foo"], []);
>     yield getSubdomainsOK(["b.a.com", "foo"], []);
> 
>     yield set("a.com", "foo", 4);
>     yield set("b.a.com", "foo", 5);
>-    yield cps.removeByDomain("b.a.com", null, makeCallback());
>+    cps.removeByDomain("b.a.com", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "foo", 4],
>     ]);
>     yield getSubdomainsOK(["a.com", "foo"], [["a.com", 4]]);
>     yield getSubdomainsOK(["b.a.com", "foo"], []);
> 
>     yield set("b.a.com", "foo", 6);
>-    yield cps.removeBySubdomain("b.a.com", null, makeCallback());
>+    cps.removeBySubdomain("b.a.com", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "foo", 4],
>     ]);
>     yield getSubdomainsOK(["a.com", "foo"], [["a.com", 4]]);
>     yield getSubdomainsOK(["b.a.com", "foo"], []);
>   },
> 
>   function privateBrowsing() {
>@@ -126,18 +137,20 @@ let tests = [
>     yield set("a.com", "bar", 2);
>     yield setGlobal("foo", 3);
>     yield setGlobal("bar", 4);
>     yield set("b.com", "foo", 5);
> 
>     let context = { usePrivateBrowsing: true };
>     yield set("a.com", "foo", 6, context);
>     yield setGlobal("foo", 7, context);
>-    yield cps.removeByDomain("a.com", context, makeCallback());
>-    yield cps.removeAllGlobals(context, makeCallback());
>+    cps.removeByDomain("a.com", context, makeCallback());
>+    yield;
>+    cps.removeAllGlobals(context, makeCallback());
>+    yield;
>     yield dbOK([
>       ["b.com", "foo", 5],
>     ]);
>     yield getOK(["a.com", "foo", context], undefined);
>     yield getOK(["a.com", "bar", context], undefined);
>     yield getGlobalOK(["foo", context], undefined);
>     yield getGlobalOK(["bar", context], undefined);
>     yield getOK(["b.com", "foo", context], 5);
>diff --git a/toolkit/components/contentprefs/tests/unit_cps2/test_removeByName.js b/toolkit/components/contentprefs/tests/unit_cps2/test_removeByName.js
>--- a/toolkit/components/contentprefs/tests/unit_cps2/test_removeByName.js
>+++ b/toolkit/components/contentprefs/tests/unit_cps2/test_removeByName.js
>@@ -7,34 +7,36 @@ function run_test() {
> }
> 
> let tests = [
> 
>   function nonexistent() {
>     yield set("a.com", "foo", 1);
>     yield setGlobal("foo", 2);
> 
>-    yield cps.removeByName("bogus", null, makeCallback());
>+    cps.removeByName("bogus", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "foo", 1],
>       [null, "foo", 2],
>     ]);
>     yield getOK(["a.com", "foo"], 1);
>     yield getGlobalOK(["foo"], 2);
>   },
> 
>   function names() {
>     yield set("a.com", "foo", 1);
>     yield set("a.com", "bar", 2);
>     yield setGlobal("foo", 3);
>     yield setGlobal("bar", 4);
>     yield set("b.com", "foo", 5);
>     yield set("b.com", "bar", 6);
> 
>-    yield cps.removeByName("foo", null, makeCallback());
>+    cps.removeByName("foo", null, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "bar", 2],
>       [null, "bar", 4],
>       ["b.com", "bar", 6],
>     ]);
>     yield getOK(["a.com", "foo"], undefined);
>     yield getOK(["a.com", "bar"], 2);
>     yield getGlobalOK(["foo"], undefined);
>@@ -50,17 +52,18 @@ let tests = [
>     yield setGlobal("bar", 4);
>     yield set("b.com", "foo", 5);
>     yield set("b.com", "bar", 6);
> 
>     let context = { usePrivateBrowsing: true };
>     yield set("a.com", "foo", 7, context);
>     yield setGlobal("foo", 8, context);
>     yield set("b.com", "bar", 9, context);
>-    yield cps.removeByName("bar", context, makeCallback());
>+    cps.removeByName("bar", context, makeCallback());
>+    yield;
>     yield dbOK([
>       ["a.com", "foo", 1],
>       [null, "foo", 3],
>       ["b.com", "foo", 5],
>     ]);
>     yield getOK(["a.com", "foo", context], 7);
>     yield getOK(["a.com", "bar", context], undefined);
>     yield getGlobalOK(["foo", context], 8);
Attachment #762453 - Attachment is obsolete: true
Attachment #762453 - Flags: review?(mak77)
Attachment #763059 - Attachment is obsolete: true
Attachment #763059 - Flags: review?(mak77)
Attachment #763061 - Attachment is obsolete: true
Attachment #763061 - Flags: review?(mak77)
Oh jeez, sorry for comment 176.
Attached patch tokens again (obsolete) — — Splinter Review 
This uses a token per browser like comment 175 says, plus the important changes mentioned in comment 106.  One other change is to make "browser" method parameters required instead of optional and falling back to gBrowser.selectedBrowser.  When they're optional it's harder to reason about async paths, and it's more bug prone.

Try looks good so far: https://tbpl.mozilla.org/?tree=Try&rev=8882b26ac4e5
Attachment #766227 - Flags: review?(mak77)
(In reply to Drew Willcoxon :adw from comment #175)
> Hmm, I'll have to think about that.  My initial impression is that queues
> sound unnecessary when you could simply keep a monotonic token per domain. 

Per browser I meant.
Comment on attachment 766227 [details] [diff] [review]
tokens again

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

I have some suggestions/questions, but the changes look doing what they are expected to do regardless my comments.

::: browser/base/content/browser-fullZoom.js
@@ +25,5 @@
>  
> +  // This maps browser outer window IDs to monotonically increasing integer
> +  // tokens.  _browserTokens[outerID] is increased each time the zoom is changed
> +  // in the browser whose outer window ID is outerID.  See _getBrowserToken.
> +  _browserTokens: {},

not very important, so you can ignore me here, though for this kind of mapping may be nicer to use a proper Map(), mostly for the API clarity of map.has(key) instead of (key in object), and map.delete(key) vs delete object[prop]
(calling this _browserTokenMap would make that nicer too)

Or you may even use a WeakMap with the dom window as the key, then you might actually stop caring about removing entries from it, since they would expire naturally. Just be sure to not use XUL objects as the keys, only DOM objects are supported.

@@ +213,3 @@
>        handleResult: function () hasPref = true,
>        handleCompletion: function () {
> +        if (token.isCurrent && !hasPref)

nit: not sure why you inverted the conditions, hasPref should still be a faster check, even if by nanoseconds :)

@@ +213,4 @@
>        handleResult: function () hasPref = true,
>        handleCompletion: function () {
> +        if (token.isCurrent && !hasPref)
> +          this._applyPrefToZoom(undefined, browser, { token: token });

I have a doubt, before any call to applyPrefToZoom you always check token.isCurrent, both here and in onLocationChange, that means the token exists and has the expected value. If you don't pass a token to _applyPrefToZoom it will invoke _getBrowserToken(aBrowser) synchronously, and I'd expect it to get the same exact value you are passing in, at that point since you just verified it is current.
What am I missing?

@@ +258,4 @@
>      // Media documents should always start at 1, and are not affected by prefs.
>      if (!aIsTabSwitch && browser.contentDocument.mozSyntheticDocument) {
>        ZoomManager.setZoomForBrowser(browser, 1);
> +      // Zoom token already incremented above.

above is a bit generic, better to say "by _ignorePendingZoomAccesses"

@@ +285,3 @@
>          }
> +        this._applyPrefToZoom(value, browser, {
> +          token: token,

ditto about my doubt, above you check !token.isCurrent, and bail out

@@ +459,5 @@
>      return {
> +      token: this._browserTokens[outerID],
> +      get isCurrent() {
> +        return !(outerID in self._browserTokens) ? false :
> +               self._browserTokens[outerID] === this.token;

If I read this correctly, you just set a missing token to 0, and .token is not a getter, so this.token is always a number >= 0. in the case you are handling here you would get undefined === number, that is already false, so the check doesn't appear useful.

@@ +475,5 @@
> +  _ignorePendingZoomAccesses: function FullZoom__ignorePendingZoomAccesses(browser) {
> +    let outerID = this._browserOuterID(browser);
> +    if (!(outerID in this._browserTokens))
> +      this._browserTokens[outerID] = 0;
> +    this._browserTokens[outerID]++;

nit: with a Map you could do: m.set(outerID, (m.get(outerID) + 1) || 0);
Attachment #766227 - Flags: review?(mak77) → review+
Attached patch tokens again 2 — — Splinter Review 
(In reply to Marco Bonardo [:mak] from comment #182)
> Or you may even use a WeakMap with the dom window as the key, then you might
> actually stop caring about removing entries from it, since they would expire
> naturally. Just be sure to not use XUL objects as the keys, only DOM objects
> are supported.

That sounded like a great idea, but unfortunately I don't think it's possible in this case.  The JS holds onto windows so it can look them up in the map later when isCurrent is called, preventing them from being collected when they're closed.

So I made it a Map (with outer window ID keys) and renamed it _browserTokenMap like you suggest.

> @@ +213,4 @@
> >        handleResult: function () hasPref = true,
> >        handleCompletion: function () {
> > +        if (token.isCurrent && !hasPref)
> > +          this._applyPrefToZoom(undefined, browser, { token: token });
> 
> I have a doubt, before any call to applyPrefToZoom you always check
> token.isCurrent, both here and in onLocationChange, that means the token
> exists and has the expected value. If you don't pass a token to
> _applyPrefToZoom it will invoke _getBrowserToken(aBrowser) synchronously,
> and I'd expect it to get the same exact value you are passing in, at that
> point since you just verified it is current.

You're right, thanks.

https://tbpl.mozilla.org/?tree=Try&rev=e9ffd7db97b9
Attachment #766227 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8057a4c1e18
Whiteboard: [test disabled][leave open]
https://hg.mozilla.org/mozilla-central/rev/f8057a4c1e18

Epic.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Depends on: 897428
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: