1335170 - Intermittent browser/base/content/test/tabs/browser_allow_process_switches_despite_related_browser.js | Check file:// URI loads in a browser that was previously for view-source - Got view-source:data:text/html,Hi, expected file:///home/worker/workspace/bu

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Treeherder Bug Filer]

Filed by: philringnalda [at] gmail.com

https://treeherder.mozilla.org/logviewer.html#?job_id=73053582&repo=autoland

https://queue.taskcluster.net/v1/task/Ldqp3KWnSc-DXSFxjqqb9g/runs/0/artifacts/public/logs/live_backing.log

Comment 1

[:Gijs (he/him)]

Looks like the test is intermittent... :-(

Comment 2

[Bob Owen (:bobowen)]

Perhaps [1] should be:

  yield BrowserTestUtils.loadURI(viewSourceBrowser, uriString);


[1] https://hg.mozilla.org/mozilla-central/file/ee975d32deb9/browser/base/content/test/tabs/browser_allow_process_switches_despite_related_browser.js#l28

Comment 3

[Bob Owen (:bobowen)]

Try push with potential fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9789be60882dcacf898a077431825826ed51675

Try push without:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9797b65dda199c8974f1d657834c3d2527050abd

Comment 4

[:Gijs (he/him)]

I think the problem is likely that the view-source page hasn't fully loaded by the time we wait for the next page to load. We should wait for that load, but I think there might not be a guarantee that it hasn't already happened by the time the waitForNewTab code returns. We might need to add code to the waitForNewTab method (to have the option of having it wait to load) if I'm right.

Comment 5

[Bob Owen (:bobowen)]

Attachment: Use BrowserTestUtils.loadURI in browser_allow_process_switches_despite_related_browser.js

How about this as a belt and braces approach?

Comment 6

[:Gijs (he/him)]

Comment on attachment 8832368 [details] [diff] [review]
Use BrowserTestUtils.loadURI in browser_allow_process_switches_despite_related_browser.js

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

If this passes on infra and doesn't produce other intermittents, that WFM. I'm not sure that'll be the case, though...

(In particular, IIRC waitForNewTab waits for an onLocationChange, and I'd expect that notification and the yield to then race with the page firing a "load" event, which might mean you end up stuck waiting for the browserLoaded promise to resolve, which will never happen because the page had already loaded when you started waiting. Similarly, I'm not convinced that we guarantee that if we start waiting for the next page load after the loadURI call resolves, we're guaranteed to catch that load (in both e10s and non-e10s). Anyway, if try is happy then all my paranoia might be just that. :-) )

Comment 7

[Bob Owen (:bobowen)]

Attachment: Use BrowserTestUtils.loadURI in browser_allow_process_switches_despite_related_browser.js

My try push looked OK, but on looking at browserLoaded again, I think you're correct I can't yield after the call that causes the load and guarantee it will work.

However, I also noticed that you can only look for certain loads, so I think this should fix the issue.

I kept the two new consts, because I think they generally made it look tidier.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8832441 [details] [diff] [review]
Use BrowserTestUtils.loadURI in browser_allow_process_switches_despite_related_browser.js

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

::: browser/base/content/test/tabs/browser_allow_process_switches_despite_related_browser.js
@@ +24,5 @@
>    const uriString = Services.io.newFileURI(dummyPage).spec;
>  
>    let viewSourceBrowser = viewSourceTab.linkedBrowser;
> +  let promiseLoad =
> +    BrowserTestUtils.browserLoaded(viewSourceBrowser, false, uriString);

Does the onLocationChange for the new tab guarantee that we will actually have switched the browser to the process that we can't load the file/http URI in? In other words, if you comment out the fix that we're testing here from the bug where we fixed this issue, does the test fail again? :-)

(asking because if we end up frequently expecting the load of the view-source URI not to have completed by the time we hit this, we wouldn't actually be guaranteeing that the test catches regressions of how this is supposed to work... )

Comment 9

[:Gijs (he/him)]

It's possible we might be able to have more of the guarantees for the issue in comment #8 if we add assertions about the relatedBrowser prop and/or processType of the tab/browser. Assuming that doesn't re-introduce intermittent-ness...

Comment 10

[Bob Owen (:bobowen)]

(In reply to :Gijs from comment #8)

> >    let viewSourceBrowser = viewSourceTab.linkedBrowser;
> > +  let promiseLoad =
> > +    BrowserTestUtils.browserLoaded(viewSourceBrowser, false, uriString);
> 
> Does the onLocationChange for the new tab guarantee that we will actually
> have switched the browser to the process that we can't load the file/http
> URI in? In other words, if you comment out the fix that we're testing here
> from the bug where we fixed this issue, does the test fail again? :-)

It does still fail.

As I understand it, the onLocationChange event is fired in the child after we have started to load, so we must have done any process setting/switching by this point.

Although, the view-source browser is using the same process as the original browser, so the only switching happens after we try and load the file: URI.
It was this switch that was failing because of the related browser.

Comment 11

[Bob Owen (:bobowen)]

Try push for current patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d87a260d8d6f0d3798c491ed1e488003ece7977b

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ef856b17a18
Fix intermittent failure in browser_allow_process_switches_despite_related_browser.js. r=Gijs

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ef856b17a18

Comment 14

[Intermittent Failures Robot]

15 failures in 733 pushes (0.02 failures/push) were associated with this bug in the last 7 days.  

Repository breakdown:
* autoland: 7
* mozilla-inbound: 5
* mozilla-central: 2
* try: 1

Platform breakdown:
* linux64: 15

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1335170&startday=2017-01-30&endday=2017-02-05&tree=all