1361992 - Investigate failures in browser_tabSwitchPrintPreview.js in e10s-multi

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[:Gijs (he/him)]

Per bug 1315042 comment 16, I pushed to try turning off the force-single-content-process pref env.

Unfortunately, it seems that causes the test to permafail on win7:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=626ac2f6a00b6e4d77668f322ea27e5807f99368&selectedJob=96561511


10:12:45     INFO -  308 INFO TEST-START | browser/base/content/test/tabs/browser_tabSwitchPrintPreview.js
10:12:51     INFO -  TEST-INFO | started process screenshot
10:12:51     INFO -  TEST-INFO | screenshot: exit 0
10:12:51     INFO -  Buffered messages logged at 10:12:45
10:12:51     INFO -  309 INFO Entering test bound
10:12:51     INFO -  310 INFO 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: "data:text/html,Should%20I%20stay%20or%20should%20I%20go?" line: 0}]
10:12:51     INFO -  Buffered messages logged at 10:12:46
10:12:51     INFO -  311 INFO 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: "data:text/html,I%20shouldn't%20be%20here!" line: 0}]
10:12:51     INFO -  312 INFO Console message: [JavaScript Error: "[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIWebBrowserPrint.printPreview]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://global/content/browser-content.js :: enterPrintPreview/< :: line 662"  data: no]"]
10:12:51     INFO -  Buffered messages finished
10:12:51    ERROR -  313 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/tabs/browser_tabSwitchPrintPreview.js | Uncaught exception - should be in print preview mode - timed out after 50 tries.
10:12:51     INFO -  314 INFO Leaving test bound
10:12:51     INFO -  GECKO(4612) | MEMORY STAT | vsize 662MB | vsizeMaxContiguous 521MB | residentFast 219MB | heapAllocated 88MB
10:12:51     INFO -  315 INFO TEST-OK | browser/base/content/test/tabs/browser_tabSwitchPrintPreview.js | took 5665ms
10:12:51     INFO -  Not taking screenshot here: see the one that was previously logged
10:12:51    ERROR -  316 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/tabs/browser_tabSwitchPrintPreview.js | Found an unexpected tab at the end of test run: data:text/html,I%20shouldn't%20be%20here! -
10:12:51     INFO -  Not taking screenshot here: see the one that was previously logged
10:12:51    ERROR -  317 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/tabs/browser_tabSwitchPrintPreview.js | Found an unexpected tab at the end of test run: data:text/html,Should%20I%20stay%20or%20should%20I%20go? -
10:12:51     INFO -  318 INFO checking window state
10:12:51     INFO -  Not taking screenshot here: see the one that was previously logged
10:12:51    ERROR -  319 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/tabs/browser_tabSwitchPrintPreview.js | Found an unexpected  at the end of test run -
10:12:51     INFO -  GECKO(4612) | must wait for focus

Mike or Bob, are you aware of there being practical restrictions on print preview in e10s-multi? Or some other obvious reason why the test doesn't work in e10s-multi?

Comment 1

[Gabor Krizsanits (INACTIVE)]

I could not reproduce it locally on my windows machine :(

Looking at http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#3505 it seems like we're trying to ensure that the print preview browser runs in the same process as the selected tab. BrowserTestUtils.withNewTab should be reliable but Blake and me fixed some weird bugs around the initial about:blank document that we load before the actual document load, there might be other edge cases that we hit here where we return sooner than the tab loaded the requested page and switched to it. Since I cannot reproduce I was wondering if anyone knows where that NS_ERROR_ILLEGAL_VALUE might come from, I could not find any method that would throw that. My best bet is that it's from glue level and it's because the ppreview tab and the documents original tab lives in different process we end up calling a function with a CPOW.

Comment 2

[Bob Owen (:bobowen)]

I don't know the print preview code too well.
I think the problem here is that we are selecting the tab at [1], before it has properly entered print preview and perhaps because of [2], we end up creating the print preview tab's browser in the process for the kURL2 tab.

Presumably we still pass down the window ID for the original kURL1 one we started print preview on and we don't find it because it doesn't exist in that process.

So, it looks like we're not quite consistently using the correct source tab in the print preview code.

If I move that line after we've waited for gInPrintPreviewMode, the test works, but it should probably work either way.

[1] https://hg.mozilla.org/mozilla-central/file/33b92d9c4056/browser/base/content/test/tabs/browser_tabSwitchPrintPreview.js#l18
[2] https://hg.mozilla.org/mozilla-central/file/33b92d9c4056/browser/base/content/browser.js#l3493

Comment 3

[:Gijs (he/him)]

(In reply to Bob Owen (:bobowen) from comment #2)
> I don't know the print preview code too well.
> I think the problem here is that we are selecting the tab at [1], before it
> has properly entered print preview and perhaps because of [2], we end up
> creating the print preview tab's browser in the process for the kURL2 tab.
> 
> Presumably we still pass down the window ID for the original kURL1 one we
> started print preview on and we don't find it because it doesn't exist in
> that process.
> 
> So, it looks like we're not quite consistently using the correct source tab
> in the print preview code.
> 
> If I move that line after we've waited for gInPrintPreviewMode, the test
> works, but it should probably work either way.
> 
> [1]
> https://hg.mozilla.org/mozilla-central/file/33b92d9c4056/browser/base/
> content/test/tabs/browser_tabSwitchPrintPreview.js#l18
> [2]
> https://hg.mozilla.org/mozilla-central/file/33b92d9c4056/browser/base/
> content/browser.js#l3493

I think the assignment to the selected tab in that test is deliberate. See bug 1301876.

Comment 4

[Bob Owen (:bobowen)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1)

> Since I cannot reproduce I
> was wondering if anyone knows where that NS_ERROR_ILLEGAL_VALUE might come
> from, I could not find any method that would throw that. 

It comes from [1] because aChildDOMWin in null and NS_ERROR_ILLEGAL_VALUE, NS_ERROR_INVALID_ARG, NS_ERROR_INVALID_POINTER,
 NS_ERROR_NULL_POINTER are all 0x80070057.

[1] https://hg.mozilla.org/mozilla-central/file/33b92d9c4056/layout/base/nsDocumentViewer.cpp#l3944

Comment 5

[Bob Owen (:bobowen)]

I have a patch that works with the existing test.

Comment 6

[Bob Owen (:bobowen)]

Attachment: Ensure that PrintPreviewListener methods all use the same source browser

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Bob Owen (:bobowen) from comment #4)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1)
> NS_ERROR_ILLEGAL_VALUE,
> NS_ERROR_INVALID_ARG, NS_ERROR_INVALID_POINTER,
>  NS_ERROR_NULL_POINTER are all 0x80070057.

Thanks, that's new to me and makes me incredibly sad :(

Comment 8

[:Gijs (he/him)]

(In reply to Bob Owen (:bobowen) from comment #6)
> Created attachment 8864560 [details] [diff] [review]
> Ensure that PrintPreviewListener methods all use the same source browser

I'm really unsure about this patch because of bug 1329220 and a similar-ish change I made earlier that that reverted because it broke people's assumptions about 'source' for print preview. I *think* this is a *different* source browser than the one at issue there, so we might be OK... but I would need to sit down for longer to be properly sure. I'll try to find time to do that tomorrow or on Saturday, but if Mike has cycles sooner and/or is more confident about reviewing this, please steal. :-)

Comment 9

[:Gijs (he/him)]

Comment on attachment 8864560 [details] [diff] [review]
Ensure that PrintPreviewListener methods all use the same source browser

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

OK, so my understanding is that this fixes the _tabBeforePrintPreview property to be set earlier (by the first caller of getSourceBrowser()), thus making sure we always print preview off the tab that was selected at the time where the print preview command was invoked, and that it's not regressing bug 1329220 (I tested). This makes sense to me, so let's go ahead and land this.

Bob, can you also remove the test's single-content-process-forcing as part of landing this?

Finally, this patch doesn't apply as-is - it looks like there's debug output in the context that isn't in the tree (anymore?), ie:

 dump("wibble: createSimplifiedBrowser\n")

:-)

Comment 10

[Bob Owen (:bobowen)]

(In reply to :Gijs from comment #9)

Thanks Gijs.

> Bob, can you also remove the test's single-content-process-forcing as part
> of landing this?

Will do.

Comment 11

[:Gijs (he/him)]

Clearing ni for Mike. :-)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2db22077f426
Ensure that PrintPreviewListener methods all use the same source browser. r=Gijs

Comment 13

[Iris Hsiao [:ihsiao]]

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

Comment 14

[Gabor Krizsanits (INACTIVE)]

Thanks guys! Gijs, do you think that this bug can be reproduced outside of our testing env.? There is a chance that we ship e10s-multi with 54 so if you're concerned about this bug we should probably uplift this patch to beta if it's safe for that.

Comment 15

[:Gijs (he/him)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> Thanks guys! Gijs, do you think that this bug can be reproduced outside of
> our testing env.?

I'm not sure. I don't think there is a way to reproduce this without a 'malicious' page trying to change the selected tab somehow (e.g. by opening new ones), but I don't know for sure.

> There is a chance that we ship e10s-multi with 54 so if
> you're concerned about this bug we should probably uplift this patch to beta
> if it's safe for that.

I think there's enough beta left that this makes sense. Bob, can you do an uplift request if you agree?

Comment 16

[Bob Owen (:bobowen)]

Attachment: * Beta Patch * : Ensure that PrintPreviewListener methods all use the same source browser

Comment 17

[Bob Owen (:bobowen)]

Comment on attachment 8865518 [details] [diff] [review]
* Beta Patch * : Ensure that PrintPreviewListener methods all use the same source browser

Approval Request Comment
[Feature/Bug causing the regression]:
Many years ago, possibly bug 544018.

[User impact if declined]:
If e10s-multi is enabled, the print preview browser can get created in the wrong process, causing print preview to fail.

[Is this code covered by automated tests?]:
The particular issue with e10s-multi is.

[Has the fix been verified in Nightly?]:
Yes. I have also reproduced manually on beta and tested the fix.

[Needs manual test from QE? If yes, steps to reproduce]:
If we don't already have print preview testing during the beta cycle, then it would be good to have some.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
Very slightly.

[Why is the change risky/not risky?]:
Although the code has changed a little bit in Fx55 the change is essentially the same and pretty simple.

[String changes made/needed]:
No.

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8865518 [details] [diff] [review]
* Beta Patch * : Ensure that PrintPreviewListener methods all use the same source browser

Fix a print preview issue with e10s-multi. Beta54+. Should be in 54 beta 7.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2a2b4755e082