829456 - Update browser-fullZoom.js to use nsIContentPrefService2

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Drew Willcoxon :adw]

browser-fullZoom.js uses the synchronous nsIContentPrefService.  It should be updated to use the async nsIContentPrefService2.

Comment 1

[Drew Willcoxon :adw]

Attachment: WIP patch

This is a straightforward conversion of browser-fullZoom.js to nsIContentPrefService2.  Marco, do you see any problems or a better approach?  I think the things that were sync but are now async are OK with being async.

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 702540 [details] [diff] [review]
WIP patch

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

it looks ok. clearly in some case the zoom may change asynchronously but we can't do much about that.

I'm not sure I like the domainOfString name though...

::: browser/base/content/browser-fullZoom.js
@@ +5,1 @@
>   */

I actually think this doesn't even need a license header cause it's included in browser.js that already has one

@@ +34,5 @@
> +   */
> +  _getGlobalValue: function FullZoom__getGlobalValue(window, callback) {
> +    // * !("_globalValue" in this) => global value not yet cached.
> +    // * this._globalValue === undefined => known not to exist.
> +    // * Otherwise, this._globalValue is a number, the value.

I'd move this documentation just before the checks, like:

// Check if global value has been cached yet.
if ("_globalValue" in this)
...
// If the value is known to not exist
// this._globalValue === undefined.
let value = undefined.
...

@@ +35,5 @@
> +  _getGlobalValue: function FullZoom__getGlobalValue(window, callback) {
> +    // * !("_globalValue" in this) => global value not yet cached.
> +    // * this._globalValue === undefined => known not to exist.
> +    // * Otherwise, this._globalValue is a number, the value.
> +    let done = (function () callback.call(this, this._globalValue)).bind(this);

this hides the code and has poor gain, I'd just hardcode it

@@ +44,5 @@
> +    let value = undefined;
> +    this._cps2.getGlobal(this.name, this._loadContextFromWindow(window), {
> +      handleResult: function (pref) value = pref.value,
> +      handleCompletion: (function () {
> +        this._globalValue = this._ensureValid(value);

please document in _ensureValid that undefined is an accepted value to indicate a known-to-not-exist value.

@@ +46,5 @@
> +      handleResult: function (pref) value = pref.value,
> +      handleCompletion: (function () {
> +        this._globalValue = this._ensureValid(value);
> +        done();
> +      }).bind(this)

nit: there should be no need to wrap function in parethesis, and looks like the currently preferred style is just function () {}.bind(this)

@@ +180,4 @@
>  
> +    let domain = this._cps2.domainOfString(gBrowser.currentURI.spec);
> +    if (aGroup) {
> +      if (aGroup == domain)

deserves some comment... domainOfString name is not that clear... maybe we should figure a better name, or just document it better.

@@ +191,5 @@
> +    // zoom should be set to the new global preference now that the global
> +    // preference has changed.
> +    let hasPref = false;
> +    this._cps2.getByDomainAndName(gBrowser.currentURI.spec, this.name,
> +                          this._loadContextFromWindow(gBrowser.contentWindow), {

feel free to go over 80, or use a temp var, to fix this indentation

@@ +196,5 @@
> +      handleResult: function () hasPref = true,
> +      handleCompletion: (function () {
> +        if (!hasPref &&
> +            gBrowser.currentURI &&
> +            this._cps2.domainOfString(gBrowser.currentURI.spec) == domain)

maybe you could cache gBrowser.currentURI before and here just check gBrowser.currentURI === cachedURI, so we avoid one parsing.

@@ +221,5 @@
> +    // zoom should be set to the default preference now that the global
> +    // preference has changed.
> +    let hasPref = false;
> +    this._cps2.getByDomainAndName(gBrowser.currentURI.spec, this.name,
> +                          this._loadContextFromWindow(gBrowser.contentWindow), {

ditto

@@ +226,5 @@
> +      handleResult: function () hasPref = true,
> +      handleCompletion: (function () {
> +        if (!hasPref &&
> +            gBrowser.currentURI &&
> +            this._cps2.domainOfString(gBrowser.currentURI.spec) == domain)

ditto

@@ +268,5 @@
> +    let loadContext = this._loadContextFromWindow(browser.contentWindow);
> +    let pref = this._cps2.getCachedByDomainAndName(aURI.spec, this.name,
> +                                                   loadContext);
> +    if (pref) {
> +      //XXXadw if pref.value === undefined, is this ok?

if the value is undefined the pref doesn't exist and _applyPrefToSetting uses the global zoom value... I think this is fine.

@@ +279,5 @@
> +      handleResult: function (resultPref) value = resultPref.value,
> +      handleCompletion: (function () {
> +        if (browser.currentURI &&
> +            this._cps2.domainOfString(browser.currentURI) ==
> +              this._cps2.domainOfString(aURI.spec))

may we just check that the uris are identical?
Btw, I don't think the first domainOfString is correct, since currentURI is not a string...

@@ +309,5 @@
>  
> +  //XXXadw
> +  // this is used in tests.  doesn't look like it'll be a problem to have it
> +  // take a callback.
> +  reset: function FullZoom_reset(callback) {

this is not just used in tests, but maybe you meant it's not a problem to have a callback for tests...
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#85

@@ +317,5 @@
> +      else
> +        ZoomManager.zoom = value;
> +      this._removePref();
> +      if (callback)
> +        Services.tm.mainThread.dispatch(callback, Ci.nsIThread.DISPATCH_NORMAL);

shouldn't this rather wait for a _removePref callback?

@@ +364,2 @@
>      }
> +    catch (ex) {}

I'd like to know what is the try catch catching... looks put there just to cover bugs... maybe check blame...

@@ +390,5 @@
> +  _loadContextFromWindow: function FullZoom__loadContextFromWindow(window) {
> +    return window.
> +           QueryInterface(Ci.nsIInterfaceRequestor).
> +           getInterface(Ci.nsIWebNavigation).
> +           QueryInterface(Ci.nsILoadContext);

nit: window.QI
           .gI
           .QI...

Comment 3

[Marco Bonardo [:mak]]

ehr, when comparing nsIURIs please use equals, I poorly put some === pseudocode

Comment 4

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [:mak] from comment #2)
> I'm not sure I like the domainOfString name though...

I called it domainOfURI at first, and then domainOfURIString, and then domainOfURISpec.  I settled on domainOfString because actually the string you pass in doesn't have to represent a URI.  But I'm not married to it.

I wish that method weren't necessary, but clearly consumers need a way to check if the domain (or more accurately the "group") of a URI matches the group in the content prefs database.

> maybe you could cache gBrowser.currentURI before and here just check
> gBrowser.currentURI === cachedURI, so we avoid one parsing.

> may we just check that the uris are identical?

I don't think so, because you may have two tabs with different URIs but the same group.  The one content pref applies to both.

> Btw, I don't think the first domainOfString is correct, since currentURI is
> not a string...

Ah, thanks.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #4)
> I called it domainOfURI at first, and then domainOfURIString, and then
> domainOfURISpec.  I settled on domainOfString because actually the string
> you pass in doesn't have to represent a URI.  But I'm not married to it.

Thinking loudly:
parseStringForDomain, extractDomain, parseDomain, parseDomainFrom, domainFixup, lookupDomain
we don't necessarily need to express the input type in the name, that's what the idl does.

Comment 6

[Drew Willcoxon :adw]

Attachment: WIP 2

This is messy and needs to be cleaned up, but I think it completely converts browser-fullZoom.js.  I'm not really happy with all the callbacks and asynchronicity.  It's bug prone and hard to follow.  There was only one test that failed and this fixes it, but there are nine other tests that use FullZoom that should be updated even if they're luckily not failing.

Comment 7

[Drew Willcoxon :adw]

Attachment: patch

This updates two tests.  There are other tests that need updating, but I'll wait for r+ on the changes to browser-fullZoom.js.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 719794 [details] [diff] [review]
patch

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

It looks ok to me.
I agree it's a bit harder to follow. In the case of tests using promises and tasks may help readability, esp when waiting for multiple events (see http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_aboutHome.js#222 for example), though in this case could mean rewriting the test and I'm not sure it's worth it.

::: browser/base/content/browser-fullZoom.js
@@ +345,3 @@
>      if (!this.siteSpecific || gInPrintPreviewMode ||
> +        content.document.mozSyntheticDocument) {
> +      this._executeSoon(callback);

why does applySettingToPref executeSoon(callback), while applyPrefToSetting directly invokes callback() in the first 3 synchronous cases?

::: browser/base/content/test/browser_bug386835.js
@@ +22,4 @@
>  
> +function waitForPossibleLocationChange(expectedTab, callback) {
> +  if (gBrowser.selectedTab == expectedTab) {
> +    Services.tm.mainThread.dispatch(callback, Ci.nsIThread.DISPATCH_NORMAL);

excuteSoon should be fine

@@ +26,5 @@
> +    return;
> +  }
> +  Services.obs.addObserver(function obs() {
> +    Services.obs.removeObserver(obs, "browser-fullZoom:locationChange");
> +    Services.tm.mainThread.dispatch(callback, Ci.nsIThread.DISPATCH_NORMAL);

ditto

::: browser/base/content/test/browser_bug719271.js
@@ +24,4 @@
>  
> +function waitForPossibleLocationChange(expectedTab, callback) {
> +  if (gBrowser.selectedTab == expectedTab) {
> +    Services.tm.mainThread.dispatch(callback, Ci.nsIThread.DISPATCH_NORMAL);

executeSoon

@@ +28,5 @@
> +    return;
> +  }
> +  Services.obs.addObserver(function obs() {
> +    Services.obs.removeObserver(obs, "browser-fullZoom:locationChange");
> +    Services.tm.mainThread.dispatch(callback, Ci.nsIThread.DISPATCH_NORMAL);

ditto

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 719794 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser-fullZoom.js b/browser/base/content/browser-fullZoom.js
>--- a/browser/base/content/browser-fullZoom.js
>+++ b/browser/base/content/browser-fullZoom.js
>@@ -1,47 +1,23 @@
>-/*
>-#ifdef 0
>- * This Source Code Form is subject to the terms of the Mozilla Public
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>- * file, You can obtain one at http://mozilla.org/MPL/2.0/.
>-#endif
>- */
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

This doesn't seem like a useful change. browser-fullZoom.js gets included in the middle of browser.js.

Comment 10

[Marco Bonardo [:mak]]

(In reply to Dão Gottwald [:dao] from comment #9)
> This doesn't seem like a useful change. browser-fullZoom.js gets included in
> the middle of browser.js.

As I said in comment 2, I think we may completely remove the license header from here, since browser.js has it and this is just a piece of it.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

The license header is most useful in the source-code form (rather than packaged form), so leaving things as-is (license header that is preprocessed out) seems like the best choice to me.

Comment 12

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [:mak] from comment #8)
> I agree it's a bit harder to follow. In the case of tests using promises and
> tasks may help readability, esp when waiting for multiple events

I just used Task.jsm for another bug, so I'll try it for these tests too.

> ::: browser/base/content/browser-fullZoom.js
> @@ +345,3 @@
> >      if (!this.siteSpecific || gInPrintPreviewMode ||
> > +        content.document.mozSyntheticDocument) {
> > +      this._executeSoon(callback);
> 
> why does applySettingToPref executeSoon(callback), while applyPrefToSetting
> directly invokes callback() in the first 3 synchronous cases?

In short, because I'm trying to avoid unnecessary turns of the event loop.

Every time a caller passes a callback to applyPrefToSetting, it subsequently calls notifyOnLocationChange, which is async.  I was trying to avoid unnecessary turns of the event loop, so applyPrefToSetting calls its callback syncly where possible.  (notifyOnLocationChange is always async because I want to guarantee observers always sync or always async behavior, and always sync isn't possible.  Making notifyOnLocationChange always async codifies that behavior.)

Every time a caller passes a callback to applySettingToPref, that callback is an external callback (i.e., a callback belonging to a browser-fullZoom caller), so the callback should be called either always syncly or always asyncly.  Async is the only option, since applySettingToPref has to call cps2.set() in one case.  applySettingToPref callers could take charge of calling the callback asyncly, but that would sometimes result in unnecessary turns of the event loop.

Comment 13

[Drew Willcoxon :adw]

Attachment: patch with tests

This is the same as the previous patch except it updates all the tests.  (The two tests in the previous patch were also changed.)  I used Task.jsm and added some helpers to head.js.

https://tbpl.mozilla.org/?tree=Try&rev=d7ff2d38fadf

Comment 14

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #12)
> Every time a caller passes a callback to applyPrefToSetting, it subsequently
> calls notifyOnLocationChange, which is async.  I was trying to avoid
> unnecessary turns of the event loop, so applyPrefToSetting calls its
> callback syncly where possible.  (notifyOnLocationChange is always async
> because I want to guarantee observers always sync or always async behavior,
> and always sync isn't possible.  Making notifyOnLocationChange always async
> codifies that behavior.)

Could you please document this somewhere in the code?

Comment 15

[Drew Willcoxon :adw]

Sure, I'll add some comments to the patch before it lands.  Or if you'd like to see them first, let me know and I can post a new patch.

Comment 16

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #15)
> Sure, I'll add some comments to the patch before it lands.  Or if you'd like
> to see them first, let me know and I can post a new patch.

no, I think I'm fine with the current patch

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 722617 [details] [diff] [review]
patch with tests

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

::: browser/base/content/browser-fullZoom.js
@@ +2,2 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

as stated in previous comment from Gavin, please add back the #ifdef 0 so the license exists here but it's not imported in browser.js

@@ +145,2 @@
>  
> +  onContentPrefChanged: function FullZoom_onContentPrefChanged(group, name, value) {

a documentation about arguments here may be good, mostly about the different behaviors based on value

I should also notice (but it's a nit) other methods in this component get "a" prefixed arguments

@@ +304,5 @@
> +   * @param aValue     The zoom level value.
> +   * @param aBrowser   The browser containing the page whose zoom level is to be
> +   *                   set.  If falsey, the currently selected browser is used.
> +   * @param aCallback  Optional.  If given, it's called when complete.  WARNING:
> +   *                   aCallback may be called synchronously or asynchronously.

as stated in previous comments, please clarify callback asynchronicity behavior.

@@ +413,5 @@
> +  _getGlobalValue: function FullZoom__getGlobalValue(window, callback) {
> +    // * !("_globalValue" in this) => global value not yet cached.
> +    // * this._globalValue === undefined => global value known not to exist.
> +    // * Otherwise, this._globalValue is a number, the global value.
> +    let done = (function () callback.call(this, this._globalValue)).bind(this);

this is just used twice and doesn't save any lines of code, I think I'd prefer it being hardcoded

::: browser/base/content/test/browser_bug719271.js
@@ +9,5 @@
>  var gTab1, gTab2, gLevel1, gLevel2;
>  
>  function test() {
> +  Task.spawn(function () {
> +    waitForExplicitFinish();

please wait before spawning

::: browser/components/privatebrowsing/test/browser/perwindow/browser_privatebrowsing_zoomrestore.js
@@ +46,1 @@
>      });

ehr, I'm not sure why the need for this change, it looks the same as the previous code but less readable, unless I misread it (fwiw may use for...of)

::: dom/interfaces/base/nsIContentPrefService2.idl
@@ +327,5 @@
> +   *             "http://example.com/foo/bar".
> +   * @return     If the given string is a valid URI, the domain of that URI is
> +   *             returned.  Otherwise, the string itself is returned.
> +   */
> +  AString extractDomain(in AString str);

I suppose could be worth to add a simple xpcshell test for this new API to toolkit/components/contentprefs/tests/unit_cps2 unless you feel like the current coverage there is good enough for it.

Comment 18

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [:mak] from comment #17)

> ::: browser/base/content/browser-fullZoom.js
> @@ +2,2 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> as stated in previous comment from Gavin, please add back the #ifdef 0 so
> the license exists here but it's not imported in browser.js

OK, but that's just silly.

> ::: browser/base/content/test/browser_bug719271.js
> @@ +9,5 @@
> >  var gTab1, gTab2, gLevel1, gLevel2;
> >  
> >  function test() {
> > +  Task.spawn(function () {
> > +    waitForExplicitFinish();
> 
> please wait before spawning

Just curious, why?

> browser/components/privatebrowsing/test/browser/perwindow/
> browser_privatebrowsing_zoomrestore.js
> @@ +46,1 @@
> >      });
> 
> ehr, I'm not sure why the need for this change, it looks the same as the
> previous code but less readable, unless I misread it (fwiw may use for...of)

Oops, this should have been

win.FullZoom.reset(function () {
  numWindows--;
  // etc.
});

I'll fix it.

> ::: dom/interfaces/base/nsIContentPrefService2.idl
> @@ +327,5 @@
> > +   *             "http://example.com/foo/bar".
> > +   * @return     If the given string is a valid URI, the domain of that URI is
> > +   *             returned.  Otherwise, the string itself is returned.
> > +   */
> > +  AString extractDomain(in AString str);
> 
> I suppose could be worth to add a simple xpcshell test for this new API to
> toolkit/components/contentprefs/tests/unit_cps2 unless you feel like the
> current coverage there is good enough for it.

Good idea, thanks.

Comment 19

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #18)
> > please wait before spawning
> 
> Just curious, why?

Makes more sense from a logical point of view, and it's what we usually do in tests.  Though, I think the result is the same.

Comment 20

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85f522534c5a
https://tbpl.mozilla.org/?tree=Try&rev=fbd349d0d57e

Comment 21

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/549474bee42c for the crime, unlikely as it seems at first glance, of causing frequent "test_bug607464.html | Pixel scrolling should have finished after one refresh driver iteration. We shouldn't be scrolling smoothly, even though the pref is set. - got 17, expected 15" failures in mochitest-1.

Comment 22

[Drew Willcoxon :adw]

There's a mouse scroll listener in browser-fullZoom.js that sets an appropriate content pref if the mouse scroll caused a zoom.  It looks like test_bug607464.html isn't the real problem: it happens to cause mouse scrolls but only for scrolling the page, not zooming.  There's one test that runs earlier, test_bug574663.html, that does do zooming with mouse scroll.  I think it's somehow interfering with the expected zoom level and throwing off the other test.  The failure isn't consistent, which makes sense with the added asynchronicity in browser-fullZoom.js's mouse scroll listener.

I had a failure run on try today with some logging that I'll look at to see if I can tell what's going on.  https://tbpl.mozilla.org/?tree=Try&rev=1bfed0a94ce6 (OS X 10.6 debug)

Comment 23

[Drew Willcoxon :adw]

Problem is something like this:

1. test_bug574663.html zooms via a mouse scroll => zoom != 1 (synchronously)
2. browser-fullZoom.js sees mouse scroll, calls _applySettingToPref
3. _applySettingToPref calls cps2.set(zoom != 1) which is async
4. test undoes the earlier zoom by zooming again => zoom == 1
5. call to cps2.set finishes, triggers _onContentPrefChanged in browser-fullZoom.js, which then sets the zoom to the value passed to cps2.set, which != 1
6. zoom remains != 1

I'll have to think about the right way to fix this...

* test_bug574663.html should wait for browser-fullZoom.js to react?
* browser-fullZoom.js is reacting poorly?
* test_bug574663.html and test_bug607464.html (the failing test) should use different URLs for their windows so that the former's messed up != 1 zoom doesn't affect the latter?

Comment 24

[Marco Bonardo [:mak]]

fwiw, imo each test should cleanup after itself, so the test changing the zoom must ensure to restore it  and not pass the handle back to the harness until everything is restored as it was when it started.

Comment 25

[Drew Willcoxon :adw]

The test isn't doing anything wrong, and in fact it does clean up after itself by undoing the zoom.  The problem is browser-fullZoom.js, so I'll try to fix it.

Comment 26

[Drew Willcoxon :adw]

Attachment: patch with tests v2

I'm running this through tryserver now, but I think this is right.  FullZoom's mouse scroll listener should suppress changing the zoom level since the mouse scroll already synchronously changed it.

The listener doesn't change it directly, though.  What happens is:

1. Mouse scroll listener fires, sets a timeout
2. Timeout fires and calls _applySettingToPref
3. _applySettingToPref calls cps2.set
4. cps2.set asynchronously finishes and notifies its observers
5. FullZoom.onContentPrefSet is called because it's an observer
6. FullZoom.onContentPrefSet sets the zoom level to the value passed to cps2.set -- but by this point the page's zoom may have already changed again, hence the test failure

This patch stops the last step from happening.

I had to modify CPS2 to call the callback first and then notify observers.  e.g., when cps2.set is done, it calls its callback and then notifies observers.  Right now it's the other way around.

So the differences between this patch and the last are the change to browser-fullZoom.js, the change to ContentPrefService2.jsm (and comments in nsIContentPrefService2.idl clarifying when observers and callbacks are called), and resulting changes in CPS2's test_observers.js.

Comment 27

[Drew Willcoxon :adw]

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=139520baaead

Comment 28

[Marco Bonardo [:mak]]

Comment on attachment 728518 [details] [diff] [review]
patch with tests v2

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

::: browser/base/content/browser-fullZoom.js
@@ +178,4 @@
>      }
> +
> +    this._globalValue = aValue === undefined ? aValue :
> +                        this._ensureValid(aValue);

indent 2 more spaces

::: browser/base/content/test/browser_bug559991.js
@@ +25,5 @@
>  
> +// -------------
> +// Test clean-up
> +function endTest() {
> +  gBrowser.removeTab(tab);

since selectTabAndWait... selects the tab, looks like you may avoid the global tab var and just use selectedTab or removeCurrentTab?

::: browser/base/content/test/browser_bug719271.js
@@ +9,5 @@
>  var gTab1, gTab2, gLevel1, gLevel2;
>  
>  function test() {
> +  Task.spawn(function () {
> +    waitForExplicitFinish();

I still suggest inverting these, for coherence with other tests

::: browser/base/content/test/head.js
@@ +267,5 @@
> +  selectTabAndWaitForLocationChange: function selectTabAndWaitForLocationChange(tab) {
> +    let deferred = Promise.defer();
> +    if (tab && gBrowser.selectedTab == tab) {
> +      deferred.resolve();
> +      return deferred.promise;

I think you may return Promise.resolve(); and define deferred later, when needed

::: toolkit/components/contentprefs/tests/unit_cps2/head.js
@@ +299,5 @@
>      cps.addObserverForName(name, obs);
>    });
>  
> +  do_execute_soon(function () {
> +    if (!dontRemove)

negating a negation is negative enough ;)
what about instead changing the third param to remove=true ?

@@ +305,5 @@
> +    next(args);
> +  });
> +}
> +
> +function wait() {

s/wait/soon/

::: toolkit/components/contentprefs/tests/unit_cps2/test_extractDomain.js
@@ +8,5 @@
> +    "http://example.com/": "example.com",
> +    "http://example.com/foo/bar/baz": "example.com",
> +    "http://subdomain.example.com/foo/bar/baz": "subdomain.example.com",
> +    "http://qix.quux.example.com/foo/bar/baz": "qix.quux.example.com",
> +    "not a url": "not a url",

what about adding some file uris too?

@@ +13,5 @@
> +  };
> +  let cps = Cc["@mozilla.org/content-pref/service;1"].
> +            getService(Ci.nsIContentPrefService2);
> +  for (let url in tests)
> +    do_check_eq(cps.extractDomain(url), tests[url]);

please brace loops

Comment 29

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74ca9737e076

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Backed out for making mochitest-browser-chrome debug runs basically perma-orange. The failure was the same as bug 807442.
https://hg.mozilla.org/integration/mozilla-inbound/rev/84719470d698

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

13:13:07     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}]
13:13:07     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Tab 1 should still be zoomed
13:13:07     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Tab 2 should still not be affected
13:13:07     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Tab 3 should have zoomed as it was loading in the background
13:13:07     INFO -  WARNING: Unable to test style tree integrity -- no content node: file ../../../layout/base/nsCSSFrameConstructor.cpp, line 8246
13:13:07     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Tab 1 should still be zoomed
13:13:07     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Tab 2 should be zoomed now
13:13:07     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Tab 3 should still be zoomed
13:13:07     INFO -  ++DOMWINDOW == 48 (0xa118ca0) [serial = 148] [outer = 0xa2205d0]
13:13:07     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Zoom should be 1 when image was loaded in the background
13:13:07     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Zoom should still be 1 when tab with image is selected
13:13:07     INFO -  WARNING: Unable to test style tree integrity -- no content node: file ../../../layout/base/nsCSSFrameConstructor.cpp, line 8246
13:13:07     INFO -  WARNING: Unable to test style tree integrity -- no content node: file ../../../layout/base/nsCSSFrameConstructor.cpp, line 8246
13:13:07     INFO -  Assertion failure: mDocument->IsXUL() || mDocument->GetReadyStateEnum() == nsIDocument::READYSTATE_INTERACTIVE || (mDocument->GetReadyStateEnum() == nsIDocument::READYSTATE_UNINITIALIZED && NS_IsAboutBlank(mDocument->GetDocumentURI())) (Bad readystate), at ../../../layout/base/nsDocumentViewer.cpp:1029
13:13:08  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | Exited with code 11 during test run
13:13:08     INFO -  INFO | automation.py | Application ran for: 0:00:49.504654
13:13:08     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmplEvkzmpidlog
13:13:15     INFO -  PROCESS-CRASH | chrome://mochitests/content/browser/browser/base/content/test/browser_bug386835.js | application crashed [@ nsDocumentViewer::LoadComplete(tag_nsresult)]
13:13:15     INFO -  Crash dump filename: /tmp/tmpPOTnf7/minidumps/578ef351-909c-3584-6784e4dd-2c406d64.dmp
13:13:15     INFO -  Operating system: Linux
13:13:15     INFO -                    0.0.0 Linux 2.6.31.5-127.fc12.i686.PAE #1 SMP Sat Nov 7 21:25:57 EST 2009 i686
13:13:15     INFO -  CPU: x86
13:13:15     INFO -       GenuineIntel family 6 model 23 stepping 10
13:13:15     INFO -       2 CPUs
13:13:15     INFO -  Crash reason:  SIGSEGV
13:13:15     INFO -  Crash address: 0x0
13:13:15     INFO -  Thread 0 (crashed)
13:13:15     INFO -   0  libxul.so!nsDocumentViewer::LoadComplete(tag_nsresult) [nsDocumentViewer.cpp:9114785edfe7 : 1021 + 0x0]
13:13:15     INFO -      eip = 0x014c53cc   esp = 0xbf95b570   ebp = 0xbf95b5f8   ebx = 0x03809c20
13:13:15     INFO -      esi = 0x00c59844   edi = 0x09c528ec   eax = 0x00000000   ecx = 0x00c5a32c
13:13:15     INFO -      edx = 0x00000000   efl = 0x00210246
13:13:15     INFO -      Found by: given as instruction pointer in context
13:13:15     INFO -   1  libxul.so!nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, tag_nsresult) [nsDocShell.cpp:9114785edfe7 : 6708 + 0xe]
13:13:15     INFO -      eip = 0x01eac40a   esp = 0xbf95b600   ebp = 0xbf95b838
13:13:15     INFO -      Found by: previous frame's frame pointer
13:13:15     INFO -   2  libxul.so!nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, tag_nsresult) [nsDocShell.cpp:9114785edfe7 : 6536 + 0x11]
13:13:15     INFO -      eip = 0x01ebc3ed   esp = 0xbf95b840   ebp = 0xbf95b928
13:13:15     INFO -      Found by: previous frame's frame pointer
13:13:15     INFO -   3  libxul.so!nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, tag_nsresult) [nsDocLoader.cpp:9114785edfe7 : 1290 + 0x25]
13:13:15     INFO -      eip = 0x01ed009e   esp = 0xbf95b930   ebp = 0xbf95b9f8
13:13:15     INFO -      Found by: previous frame's frame pointer
13:13:15     INFO -   4  libxul.so!nsDocLoader::doStopDocumentLoad(nsIRequest*, tag_nsresult) [nsDocLoader.cpp:9114785edfe7 : 870 + 0x20]
13:13:15     INFO -      eip = 0x01ed165d   esp = 0xbf95ba00   ebp = 0xbf95bad8
13:13:15     INFO -      Found by: previous frame's frame pointer
13:13:15     INFO -   5  libxul.so!nsDocLoader::DocLoaderIsEmpty(bool) [nsDocLoader.cpp:9114785edfe7 : 760 + 0xc]
13:13:15     INFO -      eip = 0x01ed199d   esp = 0xbf95bae0   ebp = 0xbf95bb38
13:13:15     INFO -      Found by: previous frame's frame pointer
13:13:15     INFO -   6  libxul.so!nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) [nsDocLoader.cpp:9114785edfe7 : 644 + 0x9]
13:13:15     INFO -      eip = 0x01ed1c84   esp = 0xbf95bb40   ebp = 0xbf95bbd8
13:13:15     INFO -      Found by: previous frame's frame pointer
13:13:15     INFO -   7  libxul.so!nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, tag_nsresult) [nsLoadGroup.cpp:9114785edfe7 : 676 + 0x18]
13:13:15     INFO -      eip = 0x012502cb   esp = 0xbf95bbe0   ebp = 0xbf95bc98
13:13:15     INFO -      Found by: previous frame's frame pointer
13:13:15     INFO -   8  libxul.so!nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, tag_nsresult) [nsLoadGroup.cpp:9114785edfe7 : 688 + 0x17]
13:13:15     INFO -      eip = 0x0125031a   esp = 0xbf95bca0   ebp = 0xbf95bd58
13:13:15     INFO -      Found by: previous frame's frame pointer

Comment 31

[Drew Willcoxon :adw]

Something happened between the try run in comment 27 (which was based off a tree last updated March 16) and the landing (March 26).  I did a bisect and found that the fatal assertion (in comment 30) is consistently triggered starting at https://hg.mozilla.org/integration/mozilla-inbound/rev/9e2bdda8c3ca (bug 716140), which landed March 24.

The assertion occurs when the test tries to navigate backward in the selected tab by calling gBrowser.goBack.  9e2bdda8c3ca has to do with image decoding, which would be involved in page loading and possibly the load-related assertion, right?  And in fact when gBrowser.goBack is called, the selected tab is a png file, although the previous page in history is an html page that doesn't contain any images.

Joe, do you know what might be going on?

(I tried sticking a setTimeout around the gBrowser.goBack call since setTimeout solves everything, and sure enough the assertion is no longer triggered, but that's not right.)

Comment 32

[Drew Willcoxon :adw]

Oh, Joe's away until April 11.  Seth, since you reviewed a lot of the patches in bug 716140, could you please take a look at comment 31?

Comment 33

[Drew Willcoxon :adw]

Henri, you added the fatal assertion in bug 775467, so maybe you could shed some light too (please see comment 31)?

Comment 34

[Seth Fowler [:seth] [:s2h]]

Drew, I'm not sure exactly what's going on just from what's posted in the bug, but I'll take a closer look tomorrow. I'm going to leave the needinfo on for now.

Comment 35

[Henri Sivonen (:hsivonen)]

(In reply to Drew Willcoxon :adw from comment #33)
> Henri, you added the fatal assertion in bug 775467, so maybe you could shed
> some light too (please see comment 31)?

Two possibilities:
 1) The change to images broke readyState handling for image docs.
 2) The readyState was already subtly wrong but timing-dependent and the changes to image decoding changed timing.

Comment 36

[Seth Fowler [:seth] [:s2h]]

Drew, FYI the patch currently doesn't apply cleanly against mozilla-central. Some of the files you're patching have disappeared.

Comment 37

[Drew Willcoxon :adw]

Attachment: unbitrotted patch

Thanks Seth.  Here's an updated patch if it helps, the last one I landed.  It still applies cleanly.  The two removed files were tests that aren't related to the failure.

Anyhow, yesterday in bug 807442 Henri made the assertion non-fatal, so I'm just going to land this as-is again, after try finishes greenly: https://tbpl.mozilla.org/?tree=Try&rev=199a823f0113

Comment 38

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b50575909c79

Try had some errors but they appear known or unrelated.
https://tbpl.mozilla.org/?tree=Try&rev=199a823f0113

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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