Closed Bug 1397365 Opened 7 years ago Closed 6 years ago

Set nodefaultsrc on the initial browser

Categories

(Firefox :: Tabbed Browser, enhancement, P3)

enhancement

Tracking

()

RESOLVED DUPLICATE of bug 1362774
Tracking Status
firefox57 --- wontfix

People

(Reporter: zbraniecki, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p3])

Attachments

(13 files, 6 obsolete files)

59 bytes, text/x-review-board-request
mconley
: review+
Details
59 bytes, text/x-review-board-request
mconley
: review+
Details
59 bytes, text/x-review-board-request
mconley
: review+
Details
59 bytes, text/x-review-board-request
mconley
: review+
Details
59 bytes, text/x-review-board-request
kmag
: review+
Details
59 bytes, text/x-review-board-request
ted
: review+
Details
59 bytes, text/x-review-board-request
asuth
: review+
Details
59 bytes, text/x-review-board-request
gasolin
: review+
Details
59 bytes, text/x-review-board-request
allstars.chh
: review-
Details
59 bytes, text/x-review-board-request
kmag
: review+
Details
59 bytes, text/x-review-board-request
mconley
: review+
Details
20.80 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
mconley
: review+
Details
In bug 1362774 we're working on removing a flicker from the new window behavior and a major reason it happens is that the tabbrowser.xml initialBrowser is triggered to load "about:blank" before anything else.

The solution suggested by :mconley is to set the `nodefaultsrc` on the browser.

This works really well and it also gives some nice performance wins (see bug 1362774 comment 65), but there are a lot of tests that rely on the old behavior.

I'd like to spin this bug to get just this one parameter in place and will need help from the test authors here.

Then, the patch in bug 1362774 will be easy.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Component: Document Navigation → Tabbed Browser
Product: Core → Firefox
Comment on attachment 8905138 [details]
Bug 1397365: Part 0 - Make the initialBrowser in tabbrowser.xml not load spurious about:blank.

This is not yet review ready, there are too many tests failing. But I'd like to get some feedback on the patch.

I'm afraid that there will be quite a number of tests to fix that just rely on the idea that they can open a browser with no url in a tab and it'll load a document in a tab.
Attachment #8905138 - Flags: review?(mconley) → feedback?(mconley)
I started with:

1) accessible/tests/mochitest/browser.js

This helper file has an optional startURL. If this is not set, the `browser-delayed-startup-finished` is not fired.

I added a workaround to open `about:mozilla` instantly, but I believe that the real fix is to make `browser-delayed-startup-finished` not depend on a tab to load something.

2) browser/components/extensions/ext-windows.js

This code has a `create` method that opens a new window and injects some observers: http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/browser/components/extensions/ext-windows.js#185

The problem is that with the patch `target.messageManage` is null here, so this throws.

Since in the code that I tested the create function was called *with* a URL (of `moz-extension://` type) I suspect that it's actually just trying to inject too early, but I'm not sure.

3) browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js

I removed tests that test the initial about:blank, since we don't have it anymore.

4) testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm

The openNewBrowserWindow method was waiting for the initial load to be completed. If we don't pass any URL, the initial load is not completed at all.

=============

Generally speaking there seem to be tests that rely on about:blank being loaded. I think that most of those tests could be easily patched by just loading `about:blank` instantly into the first tab, but for some reason that doesn't work.

If I pass "about:blank" to `Services.ww.openWindow` as an argument, it still doesn't trigger the delayed startup and other observers that are used to wait for the window to be ready. I have to pass `about:mozilla` or `about:logo` to make it work.
I suspect that there's some deeper issue with that, but I don't know how to debug it :(

NI'ing :kmag and :mconley for some early help in identifying major places that need to be updated in the platform, instead of me starting by chasing this bottom-down test-by-test.
Flags: needinfo?(mconley)
Flags: needinfo?(kmaglione+bmo)
Attachment #8905138 - Flags: review?(mconley)
Here are the remaining failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8568b8ce08246bb7a959c026f897b9e251f0fb2b&selectedJob=128993218

I (naively) hope that there's some low-level code in Gecko that has to be finetuned for this change that will make majority of those tests pass (like, the delayed startup should work), and not that each test has to be fixed manually.
Comment on attachment 8905163 [details]
Bug 1397365 - Check if a window exists when attempting to gather media telemetry.

(sorry about this, posted to the wrong bug)
Attachment #8905163 - Attachment is obsolete: true
Attachment #8905163 - Flags: review?(florian)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #0)
> This works really well and it also gives some nice performance wins (see bug
> 1362774 comment 65), but there are a lot of tests that rely on the old
> behavior.

Pretty sure at least part of the problem is that you don't load about:blank if no other URL gets loaded in a new window. You'll need to update this:

http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/browser/base/content/browser.js#1621
Dao's highlighted an important piece - we were bypassing loading about:blank because we _assumed_ the initial browser tab was going to load it anyways. How much does patching that fix things?
Flags: needinfo?(mconley) → needinfo?(gandalf)
Dao's recommendation did help a lot! Thank you!

I was able to reduce the number of failures from ~80 to 17. I do still hope that there are common causes for most of them and we can narrow it down with a bit of investigation.

Unfortunately, I'm not familiar with this part of the front end enough to spot all the other places where the code assumes that the first `about:blank` is from the initial <browser>.

I looked through all the failing tests and tried to find common causes. It seems that something about browseer-delayed-startup-finished doesn't happen, and there are many tests that fail when trying to load tabs into a private browsing window.

Here's a list and my native attempt to analyze the failures. I'm willing to spend the weekend trying to iron out the tests, but I need some pointers as to where to look for the fixes.

Can you guys look through the list below and see if you recognize anything that might be correlated with the change to the default <browser>?

Failing tests:

*) accessible/tests/mochitest/events/test_scroll.xul
*) accessible/tests/mochitest/events/test_scroll_caret.xul
*) accessible/tests/mochitest/relations/test_tabbrowser.xul
*) accessible/tests/mochitest/relations/test_ui_modalprompt.html

*) dom/workers/test/test_sharedWorker_privateBrowsing.html
*) dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html
*) dom/workers/test/serviceworkers/test_privateBrowsing.html 

*) browser/base/content/test/general/browser_bug763468_perwindowpb.js
*) browser/base/content/test/referrer/browser_referrer_middle_click.js 
*) browser/base/content/test/referrer/browser_referrer_middle_click_in_container.js
*) browser/base/content/test/referrer/browser_referrer_simple_click.js
*) browser/components/extensions/test/browser/browser_ext_commands_onCommand.js
*) browser/components/extensions/test/browser/test-oop-extensions/browser_ext_commands_onCommand.js
*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js
*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_js_uri.js
*) browser/components/extensions/test/browser/browser_ext_windows_allowScriptsToClose.js

*) devtools/client/aboutdebugging/test/browser_service_workers_not_compatible.js



===================================


I analyzed the tests to the best of my ability, and maybe this will help someone help me with them:


========  Accessibility ========

There seem to be two main issues.
The first one is that in this code: http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/accessible/tests/mochitest/browser.js#138 it seems that we never load `browser-delayed-startup-finished` if we open a window with no URL.

I worked around it by setting the defaultURL to "about:blank" and it does fix those three tests:
*) accessible/tests/mochitest/events/test_scroll_caret.xul
*) accessible/tests/mochitest/relations/test_tabbrowser.xul
*) accessible/tests/mochitest/relations/test_ui_modalprompt.html

but not this one:

*) accessible/tests/mochitest/events/test_scroll.xul


I can fix the last one by switching the default startURL to "about:mozilla". If the default URL is "about:blank", the tests hangs on this line:
http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/accessible/tests/mochitest/events/test_scroll.xul#41

If I remove it, it succeeds.


========  dom/workers ========

*) dom/workers/test/test_sharedWorker_privateBrowsing.html

The tests reports Syntax Error :
 - SyntaxError: An invalid or illegal string was specified at 
simpletestOnerror@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1652:11
OnErrorEventHandlerNonNull*@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1632:1
GECKO(7476) | JavaScript error: , line 0: SyntaxError: An invalid or illegal string was specified

I have no idea why and how is it happening. My guess is that some code gets injected into a window in SipleTest.js and it's differently concatenated.

*) dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html

13 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html | Some test failed with error TypeError: NetworkError when attempting to fetch resource. 
runTest/<@chrome://mochitests/content/chrome/dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html:160:17

*) dom/workers/test/serviceworkers/test_privateBrowsing.html 

16 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_privateBrowsing.html | Error registering worker in normal window: SecurityError: The operation is insecure. 
doTests/</<@chrome://mochitests/content/chrome/dom/workers/test/serviceworkers/test_privateBrowsing.html:73:9
17 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_privateBrowsing.html | uncaught exception - TypeError: registration is undefined at runTest@chrome://mochitests/content/chrome/dom/workers/test/serviceworkers/test_privateBrowsing.html:93:5

========  browser/base/content ========


*) browser/base/content/test/general/browser_bug763468_perwindowpb.js

25 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug763468_perwindowpb.js | URL of NewTab should be about:newtab in normal mode - Got about:blank, expected about:newtab


*) browser/base/content/test/referrer/browser_referrer_middle_click.js 
*) browser/base/content/test/referrer/browser_referrer_middle_click_in_container.js
*) browser/base/content/test/referrer/browser_referrer_simple_click.js

Those three pass if I change http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/browser/base/content/test/referrer/head.js#250

from "about:blank" to "about:mozilla", so I suspect that there's another place which skips the initial "about:blank" and shouldn't now?

*) browser/components/extensions/test/browser/browser_ext_commands_onCommand.js

No idea what's going on here. It just times out here: http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/browser/components/extensions/test/browser/browser_ext_commands_onCommand.js#238

after second command (waiting for third?)

*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js

86 INFO TEST-UNEXPECTED-FAIL | browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js | remote about:blank should have firstPartyDomain set to c2eb9047-e65e-408b-be35-ee642bdcfbd9.mozilla - "about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla" == "c2eb9047-e65e-408b-be35-ee642bdcfbd9.mozilla" - 

*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_js_uri.js

32 INFO TEST-UNEXPECTED-FAIL | browser/components/originattributes/test/browser/browser_firstPartyIsolation_js_uri.js | remote javascript: should have firstPartyDomain set to c401e5a8-73d3-444a-82ec-5f8c2e80370a.mozilla - "about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla" == "c401e5a8-73d3-444a-82ec-5f8c2e80370a.mozilla" - 
Stack trace:
resource://testing-common/content-task.js line 52 > eval:null:11
33 INFO TEST-UNEXPECTED-FAIL | browser/components/originattributes/test/browser/browser_firstPartyIsolation_js_uri.js | remote javascript: should have firstPartyDomain set to 67cec80d-a620-46bd-af98-a126b1ecab76.mozilla - "about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla" == "67cec80d-a620-46bd-af98-a126b1ecab76.mozilla" - 
Stack trace:
resource://testing-common/content-task.js line 52 > eval:null:11
34 INFO TEST-UNEXPECTED-FAIL | browser/components/originattributes/test/browser/browser_firstPartyIsolation_js_uri.js | iframe should have firstPartyDomain set to 67cec80d-a620-46bd-af98-a126b1ecab76.mozilla - "about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla" == "67cec80d-a620-46bd-af98-a126b1ecab76.mozilla" - 
Stack trace:
resource://testing-common/content-task.js line 52 > eval:null:16

*) browser/components/extensions/test/browser/browser_ext_windows_allowScriptsToClose.js

Hangs around http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/browser/components/extensions/test/browser/browser_ext_windows_allowScriptsToClose.js#44 after:

7 INFO Console message: [JavaScript Error: "TypeError: target.messageManager is null" {file: "chrome://browser/content/ext-windows.js" line: 19}]
9 INFO Console message: [JavaScript Warning: "Scripts may not close windows that were not opened by script." {file: "moz-extension://2ee6a611-6aad-42d2-849a-4e18b8e6da8c/close.js" line: 2}]

*) devtools/client/aboutdebugging/test/browser_service_workers_not_compatible.js

Hangs around http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/devtools/client/aboutdebugging/test/browser_service_workers_not_compatible.js#53
Flags: needinfo?(mconley)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gandalf)
Flags: needinfo?(florian)
Flags: needinfo?(dao+bmo)
And here's a carrot: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=7b53d362e9c5&newProject=try&newRevision=9eab5669c247d772d95fbd346c92e8e68decc33f&framework=1&showOnlyImportant=0

30-60ms win on sessionrestore, 8-10ms win on tpaint!

Also, it seems that PGO builds are timing out, so that's probably another place that somewhere waits for onload without passing URL?
Attachment #8905138 - Flags: review?(mconley)
Blocks: 1388628
Comment on attachment 8905138 [details]
Bug 1397365: Part 0 - Make the initialBrowser in tabbrowser.xml not load spurious about:blank.

https://reviewboard.mozilla.org/r/176942/#review182754

::: browser/base/content/browser.js:1622
(Diff revision 4)
>        mm.removeMessageListener("Browser:FirstPaint", onFirstPaint);
>        firstBrowserPaintDeferred.resolve();
>      });
>  
>      this._uriToLoadPromise.then(uriToLoad => {
> -      if (!uriToLoad || uriToLoad == "about:blank") {
> +      if (!uriToLoad) {

Not sure if uriToLoad can actually be null, but if so I think we should still load about:blank in that case.
The problem with browser_bug763468_perwindowpb.js is that it opens a new tab before browser-delayed-startup-finished. Replacing this load event listener with whenDelayedStartupFinished fixes that: http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/browser/base/content/test/general/head.js#216
Flags: needinfo?(dao+bmo)
> Not sure if uriToLoad can actually be null, but if so I think we should still load about:blank in that case.

It is launched with null in at least several failing tests. I'll see if switching it to about:blank fixes anything.
Switching _handleURIToLoad to set uriToLoad = "about:blank" if uriToLoad is empty fixes one test: dom/workers/test/serviceworkers/test_privateBrowsing.html 

And your suggestion fixes browser/base/content/test/general/browser_bug763468_perwindowpb.js

The current list:

*) accessible/tests/mochitest/events/test_scroll.xul
*) accessible/tests/mochitest/events/test_scroll_caret.xul
*) accessible/tests/mochitest/relations/test_tabbrowser.xul
*) accessible/tests/mochitest/relations/test_ui_modalprompt.html

*) dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html
*) dom/workers/test/serviceworkers/test_privateBrowsing.html 

*) browser/base/content/test/referrer/browser_referrer_middle_click.js 
*) browser/base/content/test/referrer/browser_referrer_middle_click_in_container.js
*) browser/base/content/test/referrer/browser_referrer_simple_click.js
*) browser/components/extensions/test/browser/browser_ext_commands_onCommand.js
*) browser/components/extensions/test/browser/test-oop-extensions/browser_ext_commands_onCommand.js
*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js
*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_js_uri.js
*) browser/components/extensions/test/browser/browser_ext_windows_allowScriptsToClose.js

*) devtools/client/aboutdebugging/test/browser_service_workers_not_compatible.js

And PGO builds.
Attachment #8905138 - Flags: review?(mconley)
Alexander, can you help us with the accessible/tests ?

I was able to fix three of them:
*) accessible/tests/mochitest/events/test_scroll_caret.xul
*) accessible/tests/mochitest/relations/test_tabbrowser.xul
*) accessible/tests/mochitest/relations/test_ui_modalprompt.html

with the patch to accessible/tests/mochitest/browser.js you can see in the patchset here. The remaining one is 
 > accessible/tests/mochitest/events/test_scroll.xul

In comment 10 I described my investigation. It's pretty weird because I do see the window opening and scrolling to the right place. It just seems that the test doesn't pick it up for some reason.
Flags: needinfo?(surkov.alexander)
Ben, you're the author of this test:
> dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html

can you take a look why would it throw with the patch in this bug? It's throwing with:

13 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html | Some test failed with error TypeError: NetworkError when attempting to fetch resource. 
runTest/<@chrome://mochitests/content/chrome/dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html:160:17

Andrea, you're the author of this test:
> dom/workers/test/test_sharedWorker_privateBrowsing.html

can you take a look at why it would fail with this patch with:

 - SyntaxError: An invalid or illegal string was specified at 
simpletestOnerror@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1652:11
OnErrorEventHandlerNonNull*@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1632:1
GECKO(7476) | JavaScript error: , line 0: SyntaxError: An invalid or illegal string was specified

Thank you.
Flags: needinfo?(bhsu)
Flags: needinfo?(amarchesini)
Gabor, you seem to recently been working on those three tests in bug 1337354:

*) browser/base/content/test/referrer/browser_referrer_middle_click.js 
*) browser/base/content/test/referrer/browser_referrer_middle_click_in_container.js
*) browser/base/content/test/referrer/browser_referrer_simple_click.js

Can you advise me on why would they be timing out with the patch attached here? Thank you
Flags: needinfo?(gkrizsanits)
:kmag, you've been the reviewer on all patches for:
*) browser/components/extensions/test/browser/browser_ext_commands_onCommand.js
*) browser/components/extensions/test/browser/test-oop-extensions/browser_ext_commands_onCommand.js

can you advise me on why it would be failing with the patch attached here? It seems to hang after the second command is tested.
Yoshi, you're the author of those two tests:
*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js
*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_js_uri.js

can you advise me on why it would be failing with the patch attached here? It seems to fail with firstPartyDomain  mismatch.
Flags: needinfo?(allstars.chh)
Fred, you wrote this test:
*) devtools/client/aboutdebugging/test/browser_service_workers_not_compatible.js

can you advise me on why it would be failing with the patch attached here? It seems to hang.
Flags: needinfo?(gasolin)
David, you recently updated the PGO set to add Speedometer. I was wondering if maybe you'll have an idea on why the patch in this bug (basically, adding nodefaultsrc to the default <browser> which makes us not load about:blank aautomatically) would make the PGO build timeout.

I've seen it here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eab5669c247d772d95fbd346c92e8e68decc33f&selectedJob=129438723

and the log on Windows and Linux looks quite similar:
 - WIndows: https://public-artifacts.taskcluster.net/E4fcpYosTuqq26DMo9yrGw/0/public/logs/live_backing.log
 - Linux: https://treeherder.mozilla.org/logviewer.html#?job_id=129438724&repo=try&lineNumber=29425

[task 2017-09-08T05:48:13.057658Z] 05:48:13     INFO -  MOZ_PGO_INSTRUMENTED=1 JARLOG_FILE=jarlog/en-US.log EXTRA_TEST_ARGS=10 /usr/local/bin/gmake -C /builds/worker/workspace/build/src/obj-firefox pgo-profile-run
[task 2017-09-08T05:48:13.061129Z] 05:48:13     INFO -  gmake[2]: Entering directory '/builds/worker/workspace/build/src/obj-firefox'
[task 2017-09-08T05:48:13.108960Z] 05:48:13     INFO -  /builds/worker/workspace/build/src/obj-firefox/_virtualenv/bin/python /builds/worker/workspace/build/src/build/pgo/profileserver.py 10
[task 2017-09-08T05:48:16.385751Z] 05:48:16     INFO -  JavaScript error: javascript:Quitter.quit(), line 1: ReferenceError: Quitter is not defined
Flags: needinfo?(dmajor)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #25)
> David, you recently updated the PGO set to add Speedometer. I was wondering
> if maybe you'll have an idea on why the patch in this bug (basically, adding
> nodefaultsrc to the default <browser> which makes us not load about:blank
> aautomatically) would make the PGO build timeout.

Maybe the Quitter content script (https://dxr.mozilla.org/mozilla-central/source/tools/quitter/contentscript.js) is not loaded if we don't load a content page?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #25)
> David, you recently updated the PGO set to add Speedometer. I was wondering
> if maybe you'll have an idea on why the patch in this bug (basically, adding
> nodefaultsrc to the default <browser> which makes us not load about:blank
> aautomatically) would make the PGO build timeout.

I don't know very much about how the browser works at these levels. The only thing I can suggest is to run a PGO build locally and watch what happens during the training (profileserver.py) phase.
Flags: needinfo?(dmajor)
Thanks! I'm pinging people somehow attached to the tests because it's really tricky to debug it test after test without experience with them .

My guess is that the "contentscript doesn't get injected" is the reason behind quite a number of tests that are still failing, but since I never worked with injecting contentscripts it's hard to verify it or fix.
Sure, let's me investigate this. (NI? myself as a reminder)
Flags: needinfo?(bhsu)
Flags: needinfo?(bhsu)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #21)
> Gabor, you seem to recently been working on those three tests in bug 1337354:
> 
> *) browser/base/content/test/referrer/browser_referrer_middle_click.js 
> *)
> browser/base/content/test/referrer/
> browser_referrer_middle_click_in_container.js
> *) browser/base/content/test/referrer/browser_referrer_simple_click.js
> 
> Can you advise me on why would they be timing out with the patch attached
> here? Thank you

Without knowing where does it time out it's hard to tell. As much as I can remember, even if you know where it times out it's not easy to tell :(

I would try to do some logging here: http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/browser/base/content/test/referrer/head.js#187

and then maybe here: http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/browser/base/content/test/referrer/head.js#138

Actually if you get to the second one you should already have some logging on the console from BrowserTestUtils.browserStopped
Maybe for about:blank the assumptions in browserStopped broke.
Flags: needinfo?(gkrizsanits)
Hi Zibi,

Thanks for your work, after investigating this a bit, I think there are some defects within the testcase itself. Currently, I've written a WIP patch and created bug 1398875 for it.
Flags: needinfo?(bhsu)
Attachment #8905138 - Flags: review?(mconley)
:mossop was able to identify the reason for referrer tests to fail, :hopang has a fix for test_devtools_serviceworker_interception.html and I was able to fix test_sharedWorker_privateBrowsing.html.

With the current patch we're down to:

*) accessible/tests/mochitest/events/test_scroll.xul

*) browser/components/extensions/test/browser/browser_ext_commands_onCommand.js
*) browser/components/extensions/test/browser/browser_ext_windows_allowScriptsToClose.js
*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js
*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_js_uri.js

*) devtools/client/aboutdebugging/test/browser_service_workers_not_compatible.js

*) PGO builds training

Requests:

*) Alexander to look into test_scroll.xul which seems to open and scroll but doesn't register it and timesout
*) :kmag to look into browser_ext_commands_onCommand.js which seems to not react to commands and times out
*) Yoshi to look into browser_firstPartyIsolation_* which seems to report wrong firstPartyDomain ID
*) Fred to look into browser_service_workers_not_compatible.js which times out
*) Florian, Mike - generic plea for help
Flags: needinfo?(amarchesini)
I think I understand what's going on with:

*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js
*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_js_uri.js

but not how to fix it.

Basically, it seems that without the patch from this bug, we're opening a new window, it loads about:blank, but doesn't define a firstPartyDomain for it.
Then we load `javascrit:1;` URI and it sets the correct firstPartyDomain.

With this patch, we, by default, don't load any URL. In order for `let win = await BrowserTestUtils.openNewBrowserWindow()` to work, we artificially add "about:blank" in BrowserTestUtils.jsm line 609 so that it's the default URL to load.

This time, since we are loading the URL, it sets the firstPartyDomain.

The result is the following mismatch:

"about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla" != "a1eca6ae-465d-464c-ab78-640d2079b300.mozilla"

I guess it based on the fact that the firstPartyDomain has `about`.

I can remove the about:blank from BrowserTestUtils.jsm, but then browser/base/content/browser.js still adds it if uriToLoad is empty in line 1620. I can remove it from there, and then BrowserTestUtils `let loadPromise = this.firstBrowserLoaded(win);` in line 652 never finishes.

Will need help with this one :(
I was able to fix devtools/client/aboutdebugging/test/browser_service_workers_not_compatible.js

Remaining:

*) accessible/tests/mochitest/events/test_scroll.xul

*) browser/components/extensions/test/browser/browser_ext_commands_onCommand.js
*) browser/components/extensions/test/browser/browser_ext_windows_allowScriptsToClose.js
*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js
*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_js_uri.js

*) PGO builds training

Requests:

*) Alexander to look into test_scroll.xul which seems to open and scroll but doesn't register it and timesout
*) :kmag to look into browser_ext_commands_onCommand.js which seems to not react to commands and times out
*) Yoshi to look into browser_firstPartyIsolation_* which seems to report wrong firstPartyDomain ID
*) Florian, Mike - generic plea for help
Flags: needinfo?(gasolin)
Attachment #8905138 - Flags: review?(mconley)
Kris: actually, I can't also understand the failure in browser/components/extensions/test/browser/browser_ext_windows_allowScriptsToClose.js
I'm not sure I can help much. In the case of browser_ext_commands_onCommand.js, we're just never getting the Alt+A key event that we dispatch after Ctrl+Up. I'm not sure why your changes would affect that, but presumably they're causing the event to get eaten somewhere, or preventing us from dispatching it.

In the case of browser_ext_windows_allowScriptsToClose.js, we're getting "TypeError: target.messageManager is null" when we try to send a message to the new tab's browser from a XULFrameLoaderCreated event, which prevents us from setting the allowScriptsToClose flag. That... is very odd, and I don't have an explanation other than the browser having been removed earlier in the event cycle.
Flags: needinfo?(kmaglione+bmo)
Thanks Kris!

My suspicion is that the two tests:


*) browser/components/extensions/test/browser/browser_ext_commands_onCommand.js
*) browser/components/extensions/test/browser/browser_ext_windows_allowScriptsToClose.js

and the PGO build, are caused by the same error.

I failed to pinpoint it yet, but it seems like we don't fire delayed-browser-startup, and/or we're not injecting the contentscript somewhere.

The PGO build test can be reproduced by just doing:

1) ./mach build
2) ./mach package
3) make -C objdir pgo-profile-run

It errors out with "Quitter is not defined".
If I remove the cmdargs from profileserver.py which calls this line, it just starts the browser with Quitter extension, but "Quitter" object is not available from inside of the browser context.

I suspect that its contentscript never gets injected.
Depends on: 1398875
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #35)
> I think I understand what's going on with:
> 
> *)
> browser/components/originattributes/test/browser/
> browser_firstPartyIsolation_aboutPages.js
> *)
Somehow the behavior of loading remote about:blank has changed, 
originally its loadFlags has LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL, but now it's gone(flags is 0),

But now I am not sure why your patch caused this.

The other test browser_firstPartyIsolation_js_uri.js failed with the same reason.
Flags: needinfo?(allstars.chh)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10)

> I worked around it by setting the defaultURL to "about:blank" and it does
> fix those three tests:
> *) accessible/tests/mochitest/events/test_scroll_caret.xul
> *) accessible/tests/mochitest/relations/test_tabbrowser.xul
> *) accessible/tests/mochitest/relations/test_ui_modalprompt.html
> 
> but not this one:
> 
> *) accessible/tests/mochitest/events/test_scroll.xul
> 
> 
> I can fix the last one by switching the default startURL to "about:mozilla".
> If the default URL is "about:blank",

what is a default URL?

> the tests hangs on this line:
> http://searchfox.org/mozilla-central/rev/
> 44c693914255638d74bcf1ec3b0bcd448dfab2fd/accessible/tests/mochitest/events/
> test_scroll.xul#41
> 
> If I remove it, it succeeds.

It appears there's a problem with EVENT_SCROLLING_START event. As a first step to approach to the problem, could you uncomment 'gA11yEventDumpToConsole = true' line and check if we do fire EVENT_SCROLLING_START event at all?
Flags: needinfo?(surkov.alexander)
I was able to fix the PGO builds and Yoshi said he'll look at the updated patch behavior again today.

Yoshi: the different "about:blank" with different flags is caused by the fact that we don't load it in <browser> anymore, but now it's the default URL loaded via `BrowserTestUtils.openNewBrowserWindow({ remote: true });`

You can change that to make the openNewBrowserWindow not load any URL by setting the second parameter to be empty:

`BrowserTestUtils.openNewBrowserWindow({ remote: true }, "");`

When I did this, the firstPartyDomain in browser/browser_firstPartyIsolation_js_uri.js became empty. I'd like the first tab to behave the same as any following tab would do (which already is loaded with nodefaultsrc="true" so doesn't start with about:blank).
Flags: needinfo?(allstars.chh)
With the latest patch, we're down to:


*) accessible/tests/mochitest/events/test_scroll.xul

*) browser/components/extensions/test/browser/browser_ext_commands_onCommand.js
*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js
*) browser/components/originattributes/test/browser/browser_firstPartyIsolation_js_uri.js
The patchset is complete now!
Flags: needinfo?(mconley)
Flags: needinfo?(florian)
Flags: needinfo?(allstars.chh)
Hi Zibi
I think the problem should be caused by the change in browser/base/content/browser.js
49 -      if (!uriToLoad || uriToLoad == "about:blank") {$                         
50 +      if (!uriToLoad) {

if I added the '|| uriToLoad == "about:blank"' back, then the test pass.

I also see the log from docshell, 
export MOZ_LOG="nsDocShellLeak:5"

GECKO(28756) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f8a9c369800 SetCurrentURI about:blank
GECKO(28756) | [Main Thread]: D/nsDocShellLeak nsDocShell[0x7f8a9c369800]: loading about:blank with flags 0x00040000
GECKO(28756) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f8a9c369800 InternalLoad about:blank

GECKO(28756) | [Main Thread]: D/nsDocShellLeak nsDocShell[0x7f8a9c369800]: loading about:blank with flags 0x00000000
GECKO(28756) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f8a9c369800 InternalLoad about:blank

See the last two lines, the same docshell loads about:blank twice.

The first load is the original one, it has the flag 'LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL'
So the first about:blank will be created with NullPrincipal, with FirstPartyDomain set to the UUID of the NullPrincipal.
And the test is testing this behavior, to see the about:blank page has the FirstPartyDomain from NullPrincipal.

Now the 2nd load comes, it doesn't have aPrincipalToInherit so it has its FirstPartyDomain set to the default value of about: pages. 
http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#11185
http://searchfox.org/mozilla-central/source/netwerk/base/nsNetUtil.h#678

This 2nd load is the load I saw in comment 42

So this is like we load any other about: page, for example, loading about:logo
TabChild will load about:blank first, then load about:logo, and this will set the firstPartyDomain to the default value of about: pages.

However if now we replace about:logo with about:blank now, and this is the case we met in browser_firstPartyIsolation_aboutPages.js, TabChild first loads about:blank, and now we load again the the same about:blank.

I am not sure the 2nd load of about:blank make sense or not, if it does, then we should update the test itself.
Comment on attachment 8907505 [details]
Bug 1397365: Part 4 - Remove nodefaultsrc from <browser> elements moved from another process.

Kris, I followed the route of removing `nodefaultsrc` when changing remoteness, and it did fix the browser_ext_windows_allowScriptsToClose.js test, but unfortunately, it broken a bunch of others:

*) browser/components/extensions/test/browser/browser_ext_sessions_restore.js
*) browser/components/extensions/test/browser/browser_ext_tabs_update_url.js
*) toolkit/content/tests/browser/browser_findbar.js
*) browser/base/content/test/performance/browser_startup.js
*) browser/base/content/test/urlbar/browser_urlbarAboutHomeLoading.js
*) toolkit/crashreporter/test/browser/browser_aboutCrashesResubmit.js

You said that alternatively, you were able to fix it by delaying the onXULFrameLoaderCreated - would you agree to use this route? If so, can you share the patch you used to delay the event?
Flags: needinfo?(kmaglione+bmo)
Attachment #8907505 - Flags: review?(kmaglione+bmo) → feedback?(kmaglione+bmo)
Ben: I incorporated your patch into the patchset to make it easier to test on try. If you want to land it in the separate bug, I'll rebase my patchset on top of it.

Yoshi: I know you said that the firstPartyDomain should not be empty. I hope that the patch attached will make it easier for you to help me identify how this test should work.
Hah, seems like my comment overlapped with yours. Thanks Yoshi, I'll look into this. Maybe we should keep the `|| uriToLoad == "about:blank"` in browser/base/content/browser.js?
smaug, could you check comment 60, now Zibi's patch will load about:blank twice, not sure if this is reasonalable.

Thanks
Flags: needinfo?(bugs)
Also, attaching the complete patchset as a single diff file for convenience.
Yoshi, I tried to revert the change to browser/base/content/browser.js and then, the firstPartyDomain tests just time out without my changes.

Can you retest against the current patchset please? I attached the combined diff.
Flags: needinfo?(allstars.chh)
Sorry, what exactly causes about:blank to be loaded twice?
Flags: needinfo?(bugs)
Comment on attachment 8907506 [details]
Bug 1397365: Part 5 - Load a data: protocol URL to quit the browser in PGO profile script.

https://reviewboard.mozilla.org/r/179208/#review184368
Attachment #8907506 - Flags: review?(ted) → review+
Comment on attachment 8907509 [details]
Bug 1397365: Part 8 - Do not expect a a11y scrolling event to be fired for the first page loaded in a new window.

I'm not sure why a first loaded page is special and why no scrolling event for it, I suspect it may be an accessibility issue, so redirecting review request to Marco for his assessment.
Attachment #8907509 - Flags: review?(surkov.alexander) → review?(mzehe)
Comment on attachment 8907509 [details]
Bug 1397365: Part 8 - Do not expect a a11y scrolling event to be fired for the first page loaded in a new window.

https://reviewboard.mozilla.org/r/179214/#review184482

Sorry, r- from me.

::: accessible/tests/mochitest/events/test_scroll.xul:40
(Diff revision 1)
>      }
>  
>      function loadTab(aURL)
>      {
>        this.eventSeq = [
> -        new asyncInvokerChecker(EVENT_DOCUMENT_LOAD_COMPLETE, currentTabDocument),
> +        new asyncInvokerChecker(EVENT_DOCUMENT_LOAD_COMPLETE, currentTabDocument)

This will cause screen readers not to pick up the scrolling to the anchor, meaning they won't know where to start reading the page loaded with an anchor. Just removing that event from checking means there *is* a problem.
Attachment #8907509 - Flags: review?(mzehe) → review-
Comment on attachment 8905138 [details]
Bug 1397365: Part 0 - Make the initialBrowser in tabbrowser.xml not load spurious about:blank.

https://reviewboard.mozilla.org/r/176942/#review184518

::: commit-message-973e8:1
(Diff revision 11)
> +Bug 1397365: Part 0 - Make the initialBrowser in tabbrowser.xml not load suprious about:blank. r?mconley

Nit - some spelling errors in here:

suprious -> spurious

"This patch unifies the behavior of the initial tab in a window with consequitive tabs" <-- I'm not fully sure what this means. Perhaps this might be clearer:

"This patch makes it so that we don't load a useless about:blank viewer in the initial tab when creating a new browser window."
Attachment #8905138 - Flags: review?(mconley) → review+
Comment on attachment 8907502 [details]
Bug 1397365: Part 1 - Make BrowserTestUtils.openNewBrowserWindow load an empty document into it by default.

https://reviewboard.mozilla.org/r/179200/#review184520

Thanks!
Attachment #8907502 - Flags: review?(mconley) → review+
Comment on attachment 8907503 [details]
Bug 1397365: Part 2 - Use about:blank as a default URL for accessibility tests.

https://reviewboard.mozilla.org/r/179202/#review184530

::: commit-message-4d229:4
(Diff revision 1)
> +tab of a new window. Since we're removing it from tabbrowser.xml, let's
> +readd it to the tests.

Nit: "readd" -> "re-add".

Perhaps to be clearer, instead of saying, "Since we're removing it from tabbrowser.xml", we should be more explicit and say, "Since we're not loading a document into the first tab of a new window by default anymore, we'll re-add the about:blank load to the tests."
Attachment #8907503 - Flags: review?(mconley) → review+
Comment on attachment 8907504 [details]
Bug 1397365: Part 3 - Fine-tune mochitest headers to wait for browser to be loaded.

https://reviewboard.mozilla.org/r/179204/#review184532

Thanks gandalf!

When this all settles out, it might be worth posting to firefox-dev so that people know to expect the initial browser to not load about:blank anymore by default.

::: commit-message-4ae9e:4
(Diff revision 1)
> +Bug 1397365: Part 3 - Fine-tune mochitest headers to wait for browser to be loaded. r?mconley
> +
> +Several header functions were not completely waiting for the new window
> +to be initialized. THe behavior was racy and the switch of the initialBrowser

Typo: "THe" -> "The", "racy" -> "race-y", "nodefaultrc" -> "nodefaultsrc".
Attachment #8907504 - Flags: review?(mconley) → review+
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #61)
> Kris, I followed the route of removing `nodefaultsrc` when changing
> remoteness, and it did fix the browser_ext_windows_allowScriptsToClose.js
> test, but unfortunately, it broken a bunch of others:
> 
> You said that alternatively, you were able to fix it by delaying the
> onXULFrameLoaderCreated - would you agree to use this route? If so, can you
> share the patch you used to delay the event?

I *would* have considered it, if removing "nodefaultsrc" were not clearly the correct solution. Removing that attribute should restore the behavior in that situation prior to your patch, so it shouldn't break any other tests.

When are you removing the attribute, exactly?
Flags: needinfo?(kmaglione+bmo)
(In reply to Marco Zehe (:MarcoZ) from comment #70)
> This will cause screen readers not to pick up the scrolling to the anchor,
> meaning they won't know where to start reading the page loaded with an
> anchor. Just removing that event from checking means there *is* a problem.

I cannot observe the scrolling with my patch, and perhaps that's why the scrolling event is not fired.

If I set the default URL in the new window in this test to be sth like "about:mozilla" I can visually observe it loading, then switching to the `http://mochi.test:8888/a11y/accessible/tests/mochitest/events/scroll.html#link1` URL *and* scrolling.

But if I don't set the `about:mozilla` URL before, it just opens the scroll.html already in position for link1.

Marco, Alexander, if I'm wrong, can you help me debug it? I don't know enough about accessibility helpers in tests to identify why both visually and from the test I get this behavior.
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(mzehe)
Comment on attachment 8907509 [details]
Bug 1397365: Part 8 - Do not expect a a11y scrolling event to be fired for the first page loaded in a new window.

Removing the r? for the accessibility patch until we confirm that it's intended.
Attachment #8907509 - Flags: review?(surkov.alexander)
Attachment #8907517 - Attachment is obsolete: true
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #76)

> But if I don't set the `about:mozilla` URL before, it just opens the
> scroll.html already in position for link1.
> 
> Marco, Alexander, if I'm wrong, can you help me debug it? I don't know
> enough about accessibility helpers in tests to identify why both visually
> and from the test I get this behavior.

We are notified about by PresShell::GoToAnchor [1], when anchor is processed, and then we fire a11y scrolling event for the anchor. Does your patch tweak the anchor processing?

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/PresShell.cpp#3194
Flags: needinfo?(surkov.alexander)
Comment on attachment 8907505 [details]
Bug 1397365: Part 4 - Remove nodefaultsrc from <browser> elements moved from another process.

https://reviewboard.mozilla.org/r/179206/#review184698
Attachment #8907505 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8907510 [details]
Bug 1397365: Part 9 - Remove the test for alt+a behavior in extensions test.

https://reviewboard.mozilla.org/r/179216/#review184700

::: commit-message-62d7d:3
(Diff revision 2)
> +Bug 1397365: Part 9 - Remove the test for alt+a behavior in extensions test. r?kmag
> +
> +It seems that with the nodefaultsrc change, the focus is now automatically

Interesting... Is that something we need to fix?
Attachment #8907510 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8907507 [details]
Bug 1397365: Part 6 - Several dom/workers tests need to wait for the environment to be ready.

https://reviewboard.mozilla.org/r/179210/#review184728

Stealing (or trying to steal) review based on discussion and investigation in #developers.

r=asuth for this plus the changes to test_privateBrowsing.html which had the same problem as test_sharedWorker_privateBrowsing.html which could result in a race where the document was "about:blank" when the register() call was made.
Attachment #8907507 - Flags: review+
Attachment #8907509 - Flags: review?(surkov.alexander)
Attachment #8907772 - Attachment is obsolete: true
Attachment #8907509 - Attachment is obsolete: true
Attachment #8907510 - Attachment is obsolete: true
I was able to remove the onCommand patch and the accessibilty patch fixing the tests by replacing `about:blank` with `data:text/html,<html></html>` as the default URL for accessibility and mochitests.

It seems that about:blank is treated specially and moves focus to the url bar. So without this patchset, the <browser> did load about:blank but it was not registered so focus was not moved to the url bar.

With this patch, the load of about:blank was via API call which moved the focus. Replacing it with an empty data: procotol document fixes the focus issues in tests.
Flags: needinfo?(mzehe)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #108)
> I was able to remove the onCommand patch and the accessibilty patch fixing
> the tests by replacing `about:blank` with `data:text/html,<html></html>` as
> the default URL for accessibility and mochitests.
> 
> It seems that about:blank is treated specially and moves focus to the url
> bar. So without this patchset, the <browser> did load about:blank but it was
> not registered so focus was not moved to the url bar.
> 
> With this patch, the load of about:blank was via API call which moved the
> focus. Replacing it with an empty data: procotol document fixes the focus
> issues in tests.

So what about this scenario:
1. Say I have a Wikipedia article with several anchors, and a link to it that specifically references one of these anchors?
2. Right now, if I open that link in a new tab, the page loads, a scroll event is fired, telling my screen reader to move its representation of the web page to that specific anchor and start reading FROM THAT POINT.
3. The way I understand the above comment, and please correct me if I'm wrong, is that the tests are now fooled into generating the event itself by adding a data URL that cures the symptom, but not the root cause. The root cause is that due to some other change, the previous sequence of event no longer fires. In the real world, this means that users of assistive technologies will get screwed once they load a URL with an anchor reference into a new tab, in that that scrolling event will no longer fire, and we will not tell the screen reader that it should start reading the web page NOT from the top, but from a specific point.

If all this rings true, I have to strongly object to this course of action. It will not make sure things worked as before, but will break users of assistive technologies by fooling the test into a situation which does not mirror the real-world scenario.

So please, answer Alex Surkov's question from comment #90.
Flags: needinfo?(gandalf)
Hi Marko,

Thank you for your feedback!

> If all this rings true, I have to strongly object to this course of action

I agree. With the help from :eeejay I was able to identify the root cause of this to be the fact that focus was placed in the urlbar instead of the document so the scroll event was not reported.
I fixed it and removed the patch for the test from the patchset as it is not needed anymore!
Flags: needinfo?(gandalf)
Mike, Kris,

I ended up separating three types of expectations the tests have for when opening a new window:

1) Tests that want just a new window, don't care about the document in it, but they wait for it to load and then inject or load the target URL
2) Tests that care that the first document is about:blank
3) Tests that care that the first document is not about:blank or data: URL.

I made the (1) the default for tests, and added (2) and (3) as a special-cases.

The reason I went for (1) is that with the change, (2) steals the focus to the URL bar. The behavior makes sense because now we do load about:blank and about:blank should place the focus in the window.

What we could do is add code to move focus to a document when it's loaded, but I believe that's outside of the scope of this bug.

Mike: The try looks greeb and I'm testing the browser on Mac and Linux now. There's one more thing that fails and it's one set of wpt tests.

I tried to debug it and so far I can't.
When I launch 
`./mach wpt testing/web-platform/tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-left.html` with this patchset, it randomly returns pass/fail which to me seems like a race condition.

I tried to replicate it by creating a custom HTML file that loads another window with dimensions and then onload reports them to the opener and in my custom test it always returns the dimensions.
On m-c it returns 11 pass 1 fail, with the patch, it returns anywhere between 3 and 11 pass and it varies from run to run.

Would you have a spare cycles to help me find the cause?
Flags: needinfo?(mconley)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #110)
> Hi Marko,
> 
> Thank you for your feedback!
> 
> > If all this rings true, I have to strongly object to this course of action
> 
> I agree. With the help from :eeejay I was able to identify the root cause of
> this to be the fact that focus was placed in the urlbar instead of the
> document so the scroll event was not reported.
> I fixed it and removed the patch for the test from the patchset as it is not
> needed anymore!

Perfect, thank you! :)
Comment on attachment 8907511 [details]
Bug 1397365: Part 8 - Change the expected behavior for firstPartyDomain javascript: url tests. .chh

https://reviewboard.mozilla.org/r/179218/#review184936

I explained in comment 60.
Attachment #8907511 - Flags: review?(allstars.chh) → review-
Flags: needinfo?(allstars.chh)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #110)
> Hi Marko,
> 
> Thank you for your feedback!
> 
> > If all this rings true, I have to strongly object to this course of action
> 
> I agree. With the help from :eeejay I was able to identify the root cause of
> this to be the fact that focus was placed in the urlbar instead of the
> document so the scroll event was not reported.
> I fixed it and removed the patch for the test from the patchset as it is not
> needed anymore!

ah, good to know that a11y tests may help to catch not a11y only regressions :)
(In reply to Yoshi Huang [:allstars.chh] from comment #60)

> I also see the log from docshell, 
> export MOZ_LOG="nsDocShellLeak:5"
> 
> GECKO(28756) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f8a9c369800
> SetCurrentURI about:blank
> GECKO(28756) | [Main Thread]: D/nsDocShellLeak nsDocShell[0x7f8a9c369800]:
> loading about:blank with flags 0x00040000
> GECKO(28756) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f8a9c369800
> InternalLoad about:blank
> 
> GECKO(28756) | [Main Thread]: D/nsDocShellLeak nsDocShell[0x7f8a9c369800]:
> loading about:blank with flags 0x00000000
> GECKO(28756) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f8a9c369800
> InternalLoad about:blank
> 
> See the last two lines, the same docshell loads about:blank twice.

I compared the m-c with the latest version of my patch and MOZ_LOG set as you did. The outcome is:

m-c log for the first test in browser_firstPartyIsolation_aboutPages.js shows:

GECKO(7205) | [Main Thread]: D/nsDocShellLeak nsDocShell[0x7f3fbf3a9000]: loading about:blank with flags 0x00040000
GECKO(7205) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f3fbf3a9000 InternalLoad about:blank
GECKO(7205) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7fbcedbb0000 SetCurrentURI about:blank
(...)
GECKO(7205) | [Main Thread]: D/nsDocShellLeak nsDocShell[0x7fbcedbb0000]: loading about:blank with flags 0x00040000
GECKO(7205) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7fbcedbb0000 InternalLoad about:blank
GECKO(7205) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7fbcedbb0000 SetCurrentURI about:blank

so it loads about:blank twice, both times with 0x00040000. I believe the second one to be the one in the document.

With the latest version of my patch I see:

GECKO(8831) | [Main Thread]: D/nsDocShellLeak nsDocShell[0x7f4e74ca5800]: loading about:blank with flags 0x00040000
GECKO(8831) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f4e74ca5800 InternalLoad about:blank
GECKO(8831) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f7c25eac000 SetCurrentURI about:blank
(...)
GECKO(8831) | [Main Thread]: D/nsDocShellLeak nsDocShell[0x7f7c25eac000]: loading data:text/html,<html></html> with flags 0x00000000
GECKO(8831) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f7c25eac000 InternalLoad data:text/html,<html></html>
GECKO(8831) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f7c25eac000 SetCurrentURI data:text/html,<html></html>

as you can see the first load is the same with the same flags, and I don't believe anything in `BrowserTestUtils.openNewBrowserWindow` affects it.

The second one is different due to the change in how we handle <browser anonid="initialBrowser"> Before my patch it was loading about:blank intrinsicly (in other words, without any `loadURI` call) and I believe that this kind of intrinsic load was setting the flag LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL.

The main goal of the patchset in this bug is to remove this intrinsic loading as it causes problems and has a performance penalty.
That means that now we create the initialBrowser *without* any document.

I believe that in result, the firstPartyDomain should not be set at all until we load any URL into it.

In the test, we have control over what is the fist URL we load into it. We can load "about:blank", nothing or "data:text/html,<html></html>" custom data url with the last being the default for `BrowserTestUtils.openNewBrowserWindow`.

In all three scenarios we *change* the behavior because even if we load "about:blank" it is a regular about:blank loaded via loadURI, rather than about:blank coming from the <browser> not having `nodefaultsrc` set.
Loading "about:blank" via loadURI does not set the `LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL` as no regular about:blank load would.

This means that without the patch, the behavior is:

1) Open new browser window with intrinsic `about:blank` (firstPartyDomain set to NullPrincipal's UUID like `UUID.mozilla`)

With the patch, the behavior in those two tests is something like:

1) Open new browser window with no URL (firstPartyDomain not set)
2) Do one of the following:
2a) Load "data:text/html,<html></html>" into the <browser> (firstPartyDomain stays "")
2b) Load "about:blank" into the <browser> (firstPartyDomain set to "about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla")
2c) Load nothing into the <browser> (firstPartyDomain stays "")


================

Because this patch changes the behavior of the initial URL loaded, it affects the firstPartyDomain.
I updated the test to reflect that.

As the domain expert and the author of this test, if you're not happy with the change to the test can you please either point out how you would like the test to work after this change?
Flags: needinfo?(allstars.chh)
Comment on attachment 8907978 [details]
Bug 1397365: Part 9 - Update several extensions tests to open new window with about:blank.

https://reviewboard.mozilla.org/r/179658/#review185210
Attachment #8907978 - Flags: review?(kmaglione+bmo) → review+
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #128)
> 
> GECKO(7205) | [Main Thread]: D/nsDocShellLeak nsDocShell[0x7f3fbf3a9000]:
> loading about:blank with flags 0x00040000
> GECKO(7205) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f3fbf3a9000
> InternalLoad about:blank
> GECKO(7205) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7fbcedbb0000
> SetCurrentURI about:blank
> (...)
> GECKO(7205) | [Main Thread]: D/nsDocShellLeak nsDocShell[0x7fbcedbb0000]:
> loading about:blank with flags 0x00040000
> GECKO(7205) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7fbcedbb0000
> InternalLoad about:blank
> GECKO(7205) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7fbcedbb0000
> SetCurrentURI about:blank
> 
> so it loads about:blank twice, both times with 0x00040000. I believe the
> second one to be the one in the document.
>
Did you notice the addresses are different? One is 0x7f3fbf3a9000 and the other is 0x7fbcedbb0000
They are different docshells.


> With the latest version of my patch I see:
> 
> GECKO(8831) | [Main Thread]: D/nsDocShellLeak nsDocShell[0x7f4e74ca5800]:
> loading about:blank with flags 0x00040000
> GECKO(8831) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f4e74ca5800
> InternalLoad about:blank
> GECKO(8831) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f7c25eac000
> SetCurrentURI about:blank
> (...)
> GECKO(8831) | [Main Thread]: D/nsDocShellLeak nsDocShell[0x7f7c25eac000]:
> loading data:text/html,<html></html> with flags 0x00000000
> GECKO(8831) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f7c25eac000
> InternalLoad data:text/html,<html></html>
> GECKO(8831) | [Main Thread]: D/nsDocShellLeak DOCSHELL 0x7f7c25eac000
> SetCurrentURI data:text/html,<html></html>
> 
Same here, they are different docshells.


> 
> The second one is different due to the change in how we handle <browser
> anonid="initialBrowser"> Before my patch it was loading about:blank
> intrinsicly (in other words, without any `loadURI` call) and I believe that
> this kind of intrinsic load was setting the flag
> LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL.
> 
> The main goal of the patchset in this bug is to remove this intrinsic
> loading as it causes problems and has a performance penalty.
> That means that now we create the initialBrowser *without* any document.
>
> I believe that in result, the firstPartyDomain should not be set at all
> until we load any URL into it.

But from the point of view of docshell, it still receives two calls to load about:blank from different callers.
It doesn't know which load is from nodefaultsrc, which is we truely want to load an URI.
More importantly, it is missing some information to decide whether it should set firstPartyDomain or not.

Perhaps your change is correct, but without explaining all the details here, it's hard to believe this is correct.
Flags: needinfo?(allstars.chh)
(In reply to Yoshi Huang [:allstars.chh] from comment #131)
> Did you notice the addresses are different? One is 0x7f3fbf3a9000 and the
> other is 0x7fbcedbb0000
> They are different docshells.

Oh, good point. Sorry, I'm new to docshell :)
I assume that the first one in both cases is a different docshell than the one we're changing and it's not relevant to the conversation.
 
> But from the point of view of docshell, it still receives two calls to load
> about:blank from different callers.

I assume only the latter matters.

> It doesn't know which load is from nodefaultsrc, which is we truely want to
> load an URI.
> More importantly, it is missing some information to decide whether it should
> set firstPartyDomain or not.

Correct, I believe that the difference is that before we did:

<browser> which intrinsically created about:blank and the way it did it triggered the LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL flag.

With the patch it inserts:

<browser nodefaultsrc="true"> which doesn't create `about:blank`.

We can load an `about:blank`, but it doesn't know about LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL

> Perhaps your change is correct, but without explaining all the details here,
> it's hard to believe this is correct.

So, I believe that I need you or someone else familiar with how firstPartyDomain is supposed to work to help me select the new behavior and update the test to it.

I spent almost a week now trying to debug it and all I can understand is that when the <browser> elements gets injected without `nodefaultsrc` attribute it creates different firstPartyDomain UUID.
Since we're changing it, I believe that those tests have to be changed as well.

I now tested the browser_firstPartyIsolation_aboutPages.js behavior using the *second* tab, instead of the initial one.

Here's my test: https://pastebin.mozilla.org/9032445

The first scenario follows the behavior the initialBrowser had before this patch. In particular it doesn't set the `nodefaultsrc` on the newly created <browser> element [0].
Since the new <browser> doesn't have the `nodefaultsrc` the firstPartyDomain UUID is as expected "UUID.mozilla".

But this is the behavior we're removing here from initialBrowser. We're removing it because it loads "about:blank" unnecessarily decreasing performance and causing the about:blank flashing in the front end UI of Firefox.

Instead we want to have the initialBrowser <browser> element have `nodefaulsrc` so that nothing is loaded until we load the first URL.

According to my understand that means that the first loaded URL into the <browser> should set the firstPartyDomain.

I don't know how and where in code this should happen and I need help from someone who understands the problem domain.
Can you help me or NI someone else who can?

[0] http://searchfox.org/mozilla-central/rev/2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/browser/base/content/tabbrowser.xml#2261
Flags: needinfo?(allstars.chh)
I'm trying another approach that maybe will avoid us having to update the tests.
Flags: needinfo?(mconley)
Flags: needinfo?(allstars.chh)
Comment on attachment 8907508 [details]
Bug 1397365: Part 7 - A client/devtools test needs to wait for browser to finish loading.

https://reviewboard.mozilla.org/r/179212/#review185292
Attachment #8907508 - Flags: review?(gasolin) → review+
on a side note, data: URI is considered as an unique opaque origin now, so this means the default for BrowserTestUtils.openNewBrowserWindow is to load a cross-origin document.
Is this intented ?
I think I found a new way to get the `nodefaultsrc` into initialBrowser without affecting tests.
In bug 1362774 I attached a patch that turns initialBrowser to be constructed programmatically in the constructor of <tabbrowser> and if the url to load is not "about:blank" it will add nodefaultsrc.

This preserves all the tests, and unifies the behavior of the first tab with all other tabs.

There's a lot of good test fixes in this bug that I'll want to salvage and land, but if all goes well, we will not need to change any test behavior for the feature to land.

I apologize for taking time from everyone involved here. I will work on getting the cleanups in this patchset landed, but I'm happy to that none of the weird test changes are needed!
> on a side note, data: URI is considered as an unique opaque origin now, so this means the default for BrowserTestUtils.openNewBrowserWindow is to load a cross-origin document.
Is this intented ?

It was not. I was just looking for a URL that will open blank but not steal focus like about:blank does.

But now I hope we won't need it. Sorry for not finding the better solution before I bothered you Yoshi :(
Priority: -- → P1
Comment on attachment 8907979 [details]
Bug 1397365: Part 10 - Define expected start pages for new windows in telemetry and sessionrestore tests.

https://reviewboard.mozilla.org/r/179660/#review186034

Thanks!
Attachment #8907979 - Flags: review?(mconley) → review+
Comment on attachment 8908323 [details]
Bug 1397365: Part 11 - Remove unnecessary boolean as a second parameter to BrowserTestUtils.openNewBrowserWindow in a test.

https://reviewboard.mozilla.org/r/179946/#review186036

Thanks!
Attachment #8908323 - Flags: review?(mconley) → review+
Thank you everyone!

I'm currently doing most of the work that originally target the bug in bug 1362774.

Both bugs are now targeting 58. My plan is to hopefully land a small 3-part patch in bug 1362774 at the beginning of 58 and then take patches from this bug that improve our tests and land them.
Gandalf, are you still working on this?
Flags: needinfo?(gandalf)
Not really. I'm blocked by bug 1401846 which may or may not be fixed as part of bug 1392352. Not sure how to disentangle it.
Assignee: gandalf → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(gandalf)
See Also: → 1425587
Whiteboard: [fxperf]
Priority: P1 → P3
Whiteboard: [fxperf] → [fxperf:p3]
Depends on: 1401846
Comment on attachment 8907505 [details]
Bug 1397365: Part 4 - Remove nodefaultsrc from <browser> elements moved from another process.

https://reviewboard.mozilla.org/r/179206/#review232302

::: browser/base/content/tabbrowser.xml:1969
(Diff revision 4)
>              }
>  
> +            // This is a temporarily solution preventing
> +            // onXULFrameLoadederCreated being called before
> +            // messageManager is ready.
> +            // See bug 1397365 for details.

Is there a bug filed for this? Referencing that would be more useful than pointing to this bug.

::: browser/base/content/tabbrowser.xml:1971
(Diff revision 4)
> +            // This is a temporarily solution preventing
> +            // onXULFrameLoadederCreated being called before
> +            // messageManager is ready.
> +            // See bug 1397365 for details.
> +            if (!keepNoDefaultSrcInInitialBrowser &&
> +                aBrowser.getAttribute("anonid") === "initialBrowser") {

You probably know this already, but bug 1442651 made browsers non-anonymous, so this needs to check aBrowser.id now.

Also, nit: we usually use == unless there's some specific reason to require type strictness.
Summary: Set nodefaultsrc on initialBrowser in tabbrowser.xml → Set nodefaultsrc on the initial browser
Flags: needinfo?(dao+bmo)
Gandalf, would you like to pick this up again? Otherwise I can take a look.
Flags: needinfo?(gandalf)
Yeah, I'd like to take a look at this, but won't have time until in 2 weeks. If you have time and want to take over, feel free to steal it :)
See Also: → 1448944
Gijs is working on this in bug 1362774.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(gandalf)
Flags: needinfo?(dao+bmo)
Keywords: stale-bug
Resolution: --- → DUPLICATE
Blocks: 1599544
No longer blocks: 1599544
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: