Closed Bug 1426599 Opened 2 years ago Closed 2 years ago

browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js fails after bug 1193394

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bevis, Assigned: arai)

References

Details

Attachments

(1 file)

New BC failure found in today's try on all desktop platform:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe67e1680fd8958d759cf41662f2db8c152d76e3

INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js | uncaught exception - TypeError: this.mBrowser is undefined at onLocationChange@chrome://browser/content/tabbrowser.xml:937:52

INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_url_overrides_newtab.js | uncaught exception - TypeError: this.mBrowser is undefined at onLocationChange@chrome://browser/content/tabbrowser.xml:937:52
Component: General → WebExtensions: Untriaged
Priority: P3 → --
Product: Firefox → Toolkit
Assignee: nobody → bob.silverberg
The test in question was updated in mid-December to include two calls to BrowserTestUtils.waitForLocationChange() [1, 2], and the API code itself was also changed to add an onLocationChange listener [3]. It seems like one of these is causing the error in RemoteWebProgress.jsm, which is being passed up from tabbrowser.xml.

I'm not sure what's causing the error "this.mBrowser is undefined" {file: "chrome://browser/content/tabbrowser.xml" line: 937}.

I'm also not sure if this is something that needs to be fixed in the test, in the API code, or elsewhere. 

[1] https://searchfox.org/mozilla-central/rev/652fbd6270de0d3ec424d2b88f8375ff546f949f/browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js#303
[2] https://searchfox.org/mozilla-central/rev/652fbd6270de0d3ec424d2b88f8375ff546f949f/browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js#424
[3] https://searchfox.org/mozilla-central/rev/652fbd6270de0d3ec424d2b88f8375ff546f949f/browser/components/extensions/ext-url-overrides.js#31
Assignee: bob.silverberg → nobody
Duplicate of this bug: 1428249
it's the similar issue to bug 1416153, removeTab is executed too early, after BrowserTestUtils.waitForLocationChange.
that is executed inside tabbrowser's onLocationChange event handler unexpectedly.

the test should wait for the next event tick before removing tab, after onLocationChange event handler.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
similar to bug 1416153, the test needs to wait for the next event tick before removing tab, after BrowserTestUtils.waitForLocationChange.
otherwise the remaining part of the test will be exected inside tabbrowser's onLocationChange handler (the same situation as bug 1416153), and the tab is removed unexpectedly.

in this case, I think it's better just wait inside the testcase itself with TestUtils.waitForTick, instead of modifying test utility functions, since this is specific to BrowserTestUtils.waitForLocationChange+BrowserTestUtils.removeTab combination, not BrowserTestUtils.waitForLocationChange itself.
Attachment #8940062 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8940062 [details] [diff] [review]
Wait for the next event tick after onLocationChange, in browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js.

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

rs=me, but re-reading the other bug, it's pretty surprising that promises can get resolved in the middle of the execution of an otherwise-synchronous JS method (because of XBL setters).

Can the same thing happen when using setAttribute, setting expando properties on a DOM node, or setting e.g. HTML5 dataset properties? If not, I guess this is yet another case where XBL removal will help us...
Attachment #8940062 - Flags: review?(gijskruitbosch+bugs) → review+
Ni'ing bgrins because comment #5 (see also bug 1416153 esp. comments 4 through 16) seems like it'd be relevant to his interests (no actual questions, just wanting to make sure you've seen this).
Flags: needinfo?(bgrinstead)
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaf2752a9ffe
Wait for the next event tick after onLocationChange, in browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaf2752a9ffea531c4093d8a3536e7cfd7241fa6
Bug 1426599 - Wait for the next event tick after onLocationChange, in browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/aaf2752a9ffe
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(In reply to :Gijs from comment #5)
> Can the same thing happen when using setAttribute, setting expando
> properties on a DOM node, or setting e.g. HTML5 dataset properties?

I think I'd quite like to know the answer to this question, actually. :-)
Flags: needinfo?(arai.unmht)
(In reply to :Gijs from comment #10)
> (In reply to :Gijs from comment #5)
> > Can the same thing happen when using setAttribute, setting expando
> > properties on a DOM node, or setting e.g. HTML5 dataset properties?
> 
> I think I'd quite like to know the answer to this question, actually. :-)

afaik the issue happens only for XBL, and it doesn't happen for those standard things.
but I'd forward the question to bevis.
Flags: needinfo?(arai.unmht) → needinfo?(btseng)
(In reply to Tooru Fujisawa [:arai] from comment #11)
> (In reply to :Gijs from comment #10)
> > (In reply to :Gijs from comment #5)
> > > Can the same thing happen when using setAttribute, setting expando
> > > properties on a DOM node, or setting e.g. HTML5 dataset properties?
> > 
> > I think I'd quite like to know the answer to this question, actually. :-)
> 
> afaik the issue happens only for XBL, and it doesn't happen for those
> standard things.
> but I'd forward the question to bevis.

Yes, this issue only happens in XBL similar to what we found in bug 1416153 comment 17 that both xpc and xbl scripts are interleaved in tabbrowser.xml.
It shall not be possible for a HTML web content to define a xpc script/listener and interact with the xpc components directly.
Flags: needinfo?(btseng)
(In reply to :Gijs from comment #6)
> Ni'ing bgrins because comment #5 (see also bug 1416153 esp. comments 4
> through 16) seems like it'd be relevant to his interests (no actual
> questions, just wanting to make sure you've seen this).

Thanks for the heads up. Hopefully now that tabbrowser isn't a XBL binding anymore we'll see this sort of thing less.
Flags: needinfo?(bgrinstead)
Flags: qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.