e10s - investigate browserDOMWindow.openURI and browser_bug537474.js failing

RESOLVED FIXED in Firefox 44



4 years ago
3 years ago


(Reporter: Gijs, Assigned: enndeakin)


(Blocks: 1 bug)

Firefox 44
Windows 8.1
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(e10s+, firefox44 fixed)



(1 attachment)

browser_bug537474.js currently fails with:

1025 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug537474.js | page loads in the current content window - Got null, expected [object Object]

in e10s mode. The comment says:

skip-if = e10s # Bug ?????? - test doesn't wait for document to be created before it checks it

but the test does:

function test() {
  var currentWin = content;
  var newWin =
    browserDOMWindow.openURI(makeURI("about:"), null,
                             Ci.nsIBrowserDOMWindow.OPEN_CURRENTWINDOW, null)
  is(newWin, currentWin, "page loads in the current content window");

I actually thought that should work. I'm not sure why openURI returns *null* rather than whatever else it might return. Bill, can you clarify?

I also wonder if this is related to bug 1101546... (if someone chooses to dupe, please DONTBUILD commit an update to the comment for this test so we don't lose track of what needs to be done to enable it on e10s)
Flags: qe-verify-
Flags: needinfo?(wmccloskey)
Flags: in-testsuite+
Flags: firefox-backlog+
tracking-e10s: ? → +
Well, browserDOMWindow.openURI always returns null in e10s. The only alternative would be to return a CPOW for the content window, but we try to avoid returning CPOWs unless the caller has specifically asked for one.

Also, we don't actually receive a CPOW from the content process until it has started loading. That's too late to return it from openURI.

Finally, we no longer use openURI in e10s anymore. We use openURIInFrame, which returns the <browser> element instead. Perhaps we could re-write the test to use that instead?
Flags: needinfo?(wmccloskey)

Comment 2

3 years ago
Created attachment 8607505 [details] [diff] [review]

The test was written to verify an aspect of openURI but not the return value specifically, so we can rework the test a bit to still call openURI.
Assignee: nobody → enndeakin
Attachment #8607505 - Flags: review?(gijskruitbosch+bugs)


3 years ago
Iteration: --- → 41.1 - May 25
Comment on attachment 8607505 [details] [diff] [review]

Review of attachment 8607505 [details] [diff] [review]:

::: browser/base/content/test/general/browser_bug537474.js
@@ +1,4 @@
> +add_task(function *() {
> +  browserDOMWindow.openURI(makeURI("about:"), null,
> +                           Ci.nsIBrowserDOMWindow.OPEN_CURRENTWINDOW, null)
> +  yield BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);

Shouldn't we create this promise before calling openURI ?
Attachment #8607505 - Flags: review?(gijskruitbosch+bugs) → review+


3 years ago
Blocks: 1130687

Comment 4

3 years ago
In bug 1130687 we need to open a new window / tab from a service worker context. In a non e10s environment there are no problems but when it is enabled OpenURI returns null as detailed in this bug. Reading comment 1 you advice to use openURIInFrame instead of openURI, however my concern is that openURI works as a kind of wrapper that that tries to open window or tab depending on the environment (preferences and other stuff). So with openURIInFrame funciton I'll have to check these thinks in my own and call openURIInFrame or openwindow2 if I want a tab or a window.

Am I wrong? Do I miss something?


3 years ago
Flags: needinfo?(wmccloskey)
Are you trying to open a window in the parent or the child? You'll have to provide more context since I don't know anything about service workers. In any case, we should talk about it in bug 1130687.
Flags: needinfo?(wmccloskey)
Neil, should the patch here be landed or is there work to do? If the latter, I could take it with some pointers on what to do next.
Flags: needinfo?(enndeakin)

Comment 7

3 years ago
I needed to address comment 3, but I'm not sure if I did that or if it didn't work properly.
Flags: needinfo?(enndeakin)
I tried the patch locally. The test case passes.
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.