1332386 - Come up with a way to make 'simplify page' not sticky when used in print preview

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[:Gijs (he/him)]

Splitting this off from bug 1329220. While there's a hacky patch to use 4 (!) browsers to accomplish this, ideally we should be able to do this without creating all that many extra browsers/docshells. Perhaps we can use createWindowlessBrowser(), or have a docshell-less document that we just feed into docshells doing print previews. Or something.

Carrying over mconley's needinfo from 1329220.

Comment 1

[Jonathan Watt [:jwatt]]

How is the "print preview" cloned document created?

Comment 2

[Matheus Longaray (:mlongaray)]

Hey Jonathan,

What we do is to re-use print preview browser as content source for its own (when re-entering on preview, e.g., user selects different orientation on preview UI). See here https://dxr.mozilla.org/mozilla-central/source/toolkit/components/printing/content/printUtils.js#205

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I'd be all for trying these windowless browsers. That sounds like a path worth exploring. I just want to avoid creating more actual tabs in the tabbrowser.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Gijs, did you have a particular idea of how we'd use windowless browsers here? Would we be replacing the simplify browser entirely?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

If it turns out that we cannot make this work with windowless browsers, we might have to try the approach from https://reviewboard.mozilla.org/r/103398/diff/1#index_header, with the 4 browsers. It's not ideal though.

Comment 6

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #4)
> Gijs, did you have a particular idea of how we'd use windowless browsers
> here? Would we be replacing the simplify browser entirely?

Not an amazingly advanced one. We have a pretty simple API right now that lets you create a windowless browser, either with or without chrome privileges - https://dxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsIAppShellService.idl#59 . I used them in bug 1338215 to remove some hacky code that used the hidden window instead.

There's a slight tradeoff here - you can't explicitly make the windowless browser you create remote. You can create a chrome one that then has a remote <browser> in it, though (which is what bug 1338215 does for the thumbnailing code). Though bz said in bug 1320124 comment #11 that we might want to make creating a remote browser directly possible anyway. Not sure how much work that would be.

In any case, not sure how important it is that the browser we create this way is remote. AIUI there's no way for a web page to force the user into print preview and no script runs in it and so it all seems relatively safe...

Does that help?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

mlongaray took the time to walk me through this problem, and I think I understand what we're dealing with (thanks mlongaray).

So, boiling it down:

1) We need a static snapshot of the document being print previewed. This is what we need to "init" from any time any of the print preview settings change.
2) We need to be able to lazily create a static snapshot of the document in simplified mode.
3) Based on the state of the "Simplify Page" checkbox, we need to have the "print preview browser" use one or the other static snapshots to back what its showing the user.

Currently, we do (1) and (2) by having the print preview browser use its own document when settings change.

Creating these static snapshots as windowless browsers seems right - though we run into some complexity, because with e10s(-multi) we need to make sure that the windowsless browsers are remote and in the right process for the source document. We might be able to get around that with a helper function, or by adding an API at a lower level.

Here's what I propose:

1) Go with the 4 browser approach that I originally pushed back on in bug 1329220. Land that on central, and try to uplift it to 54. We should make absolutely sure that the created browsers are destroyed when exiting print preview, and don't get added to SessionStore.
2) Have mlongaray work on re-jigging printUtils.js to use windowless browsers (or perhaps just a single windowless browser that contains remote browsers...). We should, at the end, only have two actual browser _tabs_ that printUtils cares about: the original source browser, and the print preview browser. The static snapshot of the source browser should be a windowless browser, and the simplified mode snapshot should also be a windowless browser.

Then, when flipping the "Simplify Page" checkbox, send a message down to the print preview browser with the outer window ID of the static snapshot we want to display.

Does that sound reasonable to you, Gijs?

Comment 8

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #7)
> mlongaray took the time to walk me through this problem, and I think I
> understand what we're dealing with (thanks mlongaray).
> 
> So, boiling it down:
> 
> 1) We need a static snapshot of the document being print previewed. This is
> what we need to "init" from any time any of the print preview settings
> change.
> 2) We need to be able to lazily create a static snapshot of the document in
> simplified mode.
> 3) Based on the state of the "Simplify Page" checkbox, we need to have the
> "print preview browser" use one or the other static snapshots to back what
> its showing the user.
> 
> Currently, we do (1) and (2) by having the print preview browser use its own
> document when settings change.
> 
> Creating these static snapshots as windowless browsers seems right - though
> we run into some complexity, because with e10s(-multi) we need to make sure
> that the windowsless browsers are remote and in the right process for the
> source document. We might be able to get around that with a helper function,
> or by adding an API at a lower level.
> 
> Here's what I propose:
> 
> 1) Go with the 4 browser approach that I originally pushed back on in bug
> 1329220. Land that on central, and try to uplift it to 54. We should make
> absolutely sure that the created browsers are destroyed when exiting print
> preview, and don't get added to SessionStore.
> 2) Have mlongaray work on re-jigging printUtils.js to use windowless
> browsers (or perhaps just a single windowless browser that contains remote
> browsers...). We should, at the end, only have two actual browser _tabs_
> that printUtils cares about: the original source browser, and the print
> preview browser. The static snapshot of the source browser should be a
> windowless browser, and the simplified mode snapshot should also be a
> windowless browser.
> 
> Then, when flipping the "Simplify Page" checkbox, send a message down to the
> print preview browser with the outer window ID of the static snapshot we
> want to display.
> 
> Does that sound reasonable to you, Gijs?

Yep, that sounds pretty good.

Did I just sign up to resurrect that 4 browser patch thing or is this something Matheus could potentially do? :-)

Comment 9

[Matheus Longaray (:mlongaray)]

I can do it :) I'll first attach that patch to this bug, and then submit a new and rebased one so we can start reviewing it.

Comment 10

[Matheus Longaray (:mlongaray)]

Attachment: Original 4-browser patch from bug 1329220

This is the original 4 browser patch from bug 1329220 first created by Gijs.

Comment 11

[Matheus Longaray (:mlongaray)]

Attachment: WIP1: Rebased version from original patch

This patch is a rebased version from the original one. The only thing added is an if-statement to avoid a null exception when removing tabs.

Comment 12

[Matheus Longaray (:mlongaray)]

Okay, so, prior to send up to MozReview, there's an issue I found when testing the patch.

If, within print preview UI, you switch real fast between portrait and landscape mode, our tab ends up crashing (it shows what I think is aboutTabCrashed.xhtml). This crash can happen even if we have never clicked on Simplify Page checkbox. It can also happen when simplify page is activated.

One thing that I tested then was deactivating e10s under preferences -> no problem there. Tabs do not crash if e10s is deactivated.

What I could catch from console was _mainly_ the following error (nothing shows up on Browser Console):

###!!! [Parent][MessageChannel] Error: (msgtype=0x2C0082,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv

Some other error messages showed up but only once - maybe it's cascading up the chain:

###!!! [Parent][MessageChannel] Error: (msgtype=0x440052,name=PContent::Msg_ParentActivated) Channel error: cannot send/recv
###!!! [Parent][MessageChannel] Error: (msgtype=0x440050,name=PContent::Msg_Activate) Channel error: cannot send/recv

Do you guys have any idea what's going on? See attached patch on comment #11.

Comment 13

[Matheus Longaray (:mlongaray)]

Update: reverted changes from patch and could confirm that the issue happens with no patch at all. If we switch modes _real_ fast between portrait and landscape, we can the same above behaviour. Issue can happen even if we have never clicked on Simplify Page checkbox. It can also happen when simplify page is activated.

Same error message is fired: ###!!! [Parent][MessageChannel] Error: (msgtype=0x2C0082,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv

Issue does not happen when e10s is off.

Should we worry about this specifically for this bug/task? Maybe file a new bug?

Comment 14

[:Gijs (he/him)]

(In reply to Matheus Longaray (:mlongaray) from comment #13)
> Update: reverted changes from patch and could confirm that the issue happens
> with no patch at all. If we switch modes _real_ fast between portrait and
> landscape, we can the same above behaviour.

> Same error message is fired: ###!!! [Parent][MessageChannel] Error:
> (msgtype=0x2C0082,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv

> Should we worry about this specifically for this bug/task? Maybe file a new
> bug?

A new bug seems appropriate. Thanks for checking. Mike Conley and/or :mrbkap might be able to help with figuring out what's crashing and why.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

> ###!!! [Parent][MessageChannel] Error: (msgtype=0x2C0082,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv

This error shows when the parent fails to contact the child because the IPC pipe has gone down, probably because the child has crashed.

same with the other [Parent][MessageChannel] errors.

Certainly we should file a new bug with the steps to reproduce this reliable crash and get a look at it. mlongaray - if you could attach gdb to the content process and get a backtrace when it goes down and post that in the bug, that would be double-y helpful! :)

Comment 16

[Matheus Longaray (:mlongaray)]

Let's proceed reviewing temporary patch then.

(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #15)
> > ###!!! [Parent][MessageChannel] Error: (msgtype=0x2C0082,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
> 
> This error shows when the parent fails to contact the child because the IPC
> pipe has gone down, probably because the child has crashed.
> 
> same with the other [Parent][MessageChannel] errors.
> 
> Certainly we should file a new bug with the steps to reproduce this reliable
> crash and get a look at it. mlongaray - if you could attach gdb to the
> content process and get a backtrace when it goes down and post that in the
> bug, that would be double-y helpful! :)

Done. See bug 1346873 :)

Comment 17

[Matheus Longaray (:mlongaray)]

Attachment: Bug 1332386 - Create extra print preview browser when using Simplify Page option.

This patch creates a new print preview browser to host the simplified cloned-document
when Simplify Page option is used on preview. Also, this patch keeps track of what browser
should be presented whether simplify page checkbox is checked.

Review commit: https://reviewboard.mozilla.org/r/119812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119812/

Comment 18

[:Gijs (he/him)]

Comment on attachment 8846804 [details]
Bug 1332386 - Create extra print preview browser when using Simplify Page option.

https://reviewboard.mozilla.org/r/119812/#review122180

This looks OK from the code, but when I do the following:

1) load a BBC news article, e.g. http://www.bbc.co.uk/news/uk-39272261
2) open print preview
3) click landscape, or change the scaling factor

The page reloads but the settings remain the same as before in the toolbar - but the content displayed does change. So 'portrait' is always selected, and after switching to landscape you can never switch back. Likewise, turning off 'simplify page' doesn't seem to work. So something is still missing, I think. :-(

::: commit-message-f9362:4
(Diff revision 1)
> +when Simplify Page option is used on preview. Also, this patch keeps track of what browser
> +should be presented whether simplify page checkbox is checked.

English nit on the commit msg:

"Also, this patch keeps track of what browser should be presented, based on whether the 'Simplify page' checkbox is checked."

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8846804 [details]
Bug 1332386 - Create extra print preview browser when using Simplify Page option.

https://reviewboard.mozilla.org/r/119812/#review122540

Clearing review until https://bugzilla.mozilla.org/show_bug.cgi?id=1332386#c18 issues are resolved.

Comment 20

[Matheus Longaray (:mlongaray)]

(In reply to :Gijs from comment #18)
> Comment on attachment 8846804 [details]
> Bug 1332386 - Create extra print preview browser when using Simplify Page
> option.
> 
> https://reviewboard.mozilla.org/r/119812/#review122180
> 
> This looks OK from the code, but when I do the following:
> 
> 1) load a BBC news article, e.g. http://www.bbc.co.uk/news/uk-39272261
> 2) open print preview
> 3) click landscape, or change the scaling factor
> 
> The page reloads but the settings remain the same as before in the toolbar -
> but the content displayed does change. So 'portrait' is always selected, and
> after switching to landscape you can never switch back. Likewise, turning
> off 'simplify page' doesn't seem to work. So something is still missing, I
> think. :-(

Ow, that's weird. I had tested it before and did not see any issue like this. So _today_ I rebased the patch against current Nightly, built it, and I'm still not able to reproduce the problem. Is there anything else I could do to reproduce it?

Comment 21

[:Gijs (he/him)]

(In reply to Matheus Longaray (:mlongaray) from comment #20)
> (In reply to :Gijs from comment #18)
> > Comment on attachment 8846804 [details]
> > Bug 1332386 - Create extra print preview browser when using Simplify Page
> > option.
> > 
> > https://reviewboard.mozilla.org/r/119812/#review122180
> > 
> > This looks OK from the code, but when I do the following:
> > 
> > 1) load a BBC news article, e.g. http://www.bbc.co.uk/news/uk-39272261
> > 2) open print preview
> > 3) click landscape, or change the scaling factor
> > 
> > The page reloads but the settings remain the same as before in the toolbar -
> > but the content displayed does change. So 'portrait' is always selected, and
> > after switching to landscape you can never switch back. Likewise, turning
> > off 'simplify page' doesn't seem to work. So something is still missing, I
> > think. :-(
> 
> Ow, that's weird. I had tested it before and did not see any issue like
> this. So _today_ I rebased the patch against current Nightly, built it, and
> I'm still not able to reproduce the problem. Is there anything else I could
> do to reproduce it?

Are you testing against a clean profile? You can use

./mach run -P

to bring up the profile manager to create/delete temp profiles to be extra sure. The other thing that might explain the difference between our results is whether e10s is on or off - you can toggle it from the options (Tools > Options on Windows).

I can't re-test very easily today because bug 1348825 broke my build (sort of). I'll try and get a working build for tomorrow and then re-test, if you don't find out what's broken today I can try to dive in a bit more with a debugger and see if I can unearth what's up.

Comment 22

[Matheus Longaray (:mlongaray)]

Thanks for your instructions, Gijs.

So, tested against two brand new and clean profiles: one where e10s is on, and another where e10s is off. No success, could not reproduce the issue when toggling settings in the toolbar :(.

I'm currently developing on Linux by the way. Perhaps that's the point where our builds don't match?

Comment 23

[:Gijs (he/him)]

(In reply to Matheus Longaray (:mlongaray) from comment #22)
> Thanks for your instructions, Gijs.
> 
> So, tested against two brand new and clean profiles: one where e10s is on,
> and another where e10s is off. No success, could not reproduce the issue
> when toggling settings in the toolbar :(.
> 
> I'm currently developing on Linux by the way. Perhaps that's the point where
> our builds don't match?

Hmm, I now noticed that this is a problem with the test machine I'm using. Outside of e10s, opening print preview fails with the alert "The selected printer could not be found.". Within e10s, print preview opens but has the bugs I noted earlier. Doesn't happen on 53 beta. I'll file a separate bug for that issue once I figure out what causes it... In any case, I haven't figured out how to not have this problem - it happens on clean profiles, and also happens without this patch. It seems to be caused by this Windows 10 machine thinking it has access to an HP printer which is no longer installed (our printing needs changed, and so we changed printers...).

Sorry for the confusion!

Maybe Mike can do another review? As I noted in comment #18, the code looks OK to me. An automated test would be good, I think...

Comment 24

[:Gijs (he/him)]

OK, it seems the only reason I'm not seeing issues on beta is that my regular profile has at some point started using Microsoft's "print to pdf" virtual printer, and with that printer selected my issues go away. I just tested the patch again with that done, and it looks fine to me. Mike should still have a proper review seeing as I wrote the patch's predecessor :-)

Comment 25

[Matheus Longaray (:mlongaray)]

Comment on attachment 8846804 [details]
Bug 1332386 - Create extra print preview browser when using Simplify Page option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119812/diff/1-2/

Comment 26

[Matheus Longaray (:mlongaray)]

Sending rebased patch as well as commit message fix. Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=54c33421b4d4e10b536491aba0552a85e93ae4b8

Comment 27

[Matheus Longaray (:mlongaray)]

I think we're good with the last try run (only two intermittent failures).

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Will do another review as soon as I get out from these meetings. Thanks for the patch! Hopefully a review soon!

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8846804 [details]
Bug 1332386 - Create extra print preview browser when using Simplify Page option.

https://reviewboard.mozilla.org/r/119812/#review128702

Hey mlongaray, finally got this. I'm so, so sorry it took so long to reach this point in my queue. :(

I have some questions! See below.

::: browser/base/content/browser.js:3520
(Diff revision 2)
> +      }
>      }
> -    gBrowser.removeTab(this._printPreviewTab);
>      gBrowser.deactivatePrintPreviewBrowsers();
> -    this._printPreviewTab = null;
> +    for (let tabProp of tabsToRemove) {
> +      this[tabProp] = null;

Can't this be moved into the above loop?

::: browser/base/content/tabbrowser.xml:3323
(Diff revision 2)
>          <getter>
>            return this.mCurrentTab;
>          </getter>
>          <setter>
>            <![CDATA[
> -          if (gNavToolbox.collapsed) {
> +          if (gNavToolbox.collapsed && !this._allowTabChange) {

:( Alright. Can't really think of a better way around this right now.

::: toolkit/components/printing/content/printUtils.js:517
(Diff revision 2)
>    setSimplifiedMode(shouldSimplify) {
>      this._shouldSimplify = shouldSimplify;
>    },
>  
> +  _ppBrowsers: new Set(),
> +  _currentPPBrowser: null,

Can you please add some documentation about what this variable represents? What does it mean to be "current"?

::: toolkit/components/printing/content/printUtils.js:536
(Diff revision 2)
> +    // preview, we will want to run reader mode against the 'normal' print
> +    // preview browser's content:
> +    let oldPPBrowser = null;
> +    let changingPrintPreviewBrowsers = false;
> +    if (ppBrowser != this._currentPPBrowser) {
> +      changingPrintPreviewBrowsers = true;

Instead of holding this variable in the closure, can we pass it with the Print:Preview:Enter message? Then, browser-content.js can send it back to us in the Print:Preview:Entered message, and we can do the re-initialization.

::: toolkit/components/printing/content/printUtils.js:577
(Diff revision 2)
>          // Here, we send down a message to simplified browser in order to parse
>          // the original page. After we have parsed it, content will tell parent
>          // that the document is ready for print previewing.
>          spMM.sendAsyncMessage("Printing:Preview:ParseDocument", {
>            URL: this._originalURL,
> -          windowID: this._sourceBrowser.outerWindowID,
> +          windowID: oldPPBrowser.outerWindowID,

Shouldn't this always be the source browser?

Comment 30

[Matheus Longaray (:mlongaray)]

Comment on attachment 8846804 [details]
Bug 1332386 - Create extra print preview browser when using Simplify Page option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119812/diff/2-3/

Comment 31

[Matheus Longaray (:mlongaray)]

Comment on attachment 8846804 [details]
Bug 1332386 - Create extra print preview browser when using Simplify Page option.

https://reviewboard.mozilla.org/r/119812/#review128702

> Instead of holding this variable in the closure, can we pass it with the Print:Preview:Enter message? Then, browser-content.js can send it back to us in the Print:Preview:Entered message, and we can do the re-initialization.

Done. I also added a null-check when verifying if print preview browsers are being changed (when we first enter on preview, _currentPPBrowser variable is null).

> Shouldn't this always be the source browser?

We are re-setting "sourceBrowser" when re-entering on preview (see line 233). If Simplify Page option was clicked, sourceBrowser would be an about:printpreview tab holding a blank document up to that point. Parsing a blank document with Reader Mode would not return an article able to be injected into the DOM.
In this case, since we have just changed preview browsers, oldPPBrowser represents the content from print preview browser (as currentPPBrowser is the simplified print preview browser).

Comment 32

[Matheus Longaray (:mlongaray)]

Hey Mike, just so you know, I've already rebased this patch against latest mozilla-central changes. Also, as per Gijs suggestion, I added an automated test to verify the browser switching thing when we click on Simplify.

Comment 33

[Matheus Longaray (:mlongaray)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e13e6163086e

Comment 34

[Matheus Longaray (:mlongaray)]

Automated test passed on mochitest suites for Linux, but did not pass on 'lint opt'. Is there something missing? See https://treeherder.mozilla.org/#/jobs?repo=try&revision=e13e6163086e&group_state=expanded&selectedJob=90100443

Comment 35

[Mike Conley (:mconley) (:⚙️)]

(In reply to Matheus Longaray (:mlongaray) from comment #34)
> Automated test passed on mochitest suites for Linux, but did not pass on
> 'lint opt'. Is there something missing? See
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e13e6163086e&group_state=expanded&selectedJob=90100443

What happens if you were to drop this into a .eslintrc.js file in toolkit/components/printing/test:


"use strict";

module.exports = {
  "extends": [
    "plugin:mozilla/browser-test"
  ]
};


Does that help? Are you able to make ESLint fail locally? Use ./mach eslint --setup to get it set up, and then run it against your test with:

./mach eslint <path to test>

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8846804 [details]
Bug 1332386 - Create extra print preview browser when using Simplify Page option.

https://reviewboard.mozilla.org/r/119812/#review131524

::: toolkit/components/printing/tests/browser.ini:6
(Diff revision 3)
>  [browser_page_change_print_original.js]
>  skip-if = os == "mac"
> +
> +[browser_preview_switch_print_selected.js]
> +support-files =
> +	simplifyArticleSample.html

4 space indent here please

::: toolkit/components/printing/tests/browser_preview_switch_print_selected.js:1
(Diff revision 3)
> + /**
> +  * Verify if we correctly switch print preview browsers based on whether
> +  * Simplify Page checkbox is checked.
> +  */

Thanks for the documentation here! No spaces before this comment block though.

::: toolkit/components/printing/tests/browser_preview_switch_print_selected.js:10
(Diff revision 3)
> +  // Ensure we have the simplify page preference set
> +	Services.prefs.setBoolPref("print.use_simplify_page", true);
> +});

For this, best to use SpecialPowers.pushPrefEnv so that the pref change is undone automatically after the test completes.

Example: http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/browser/components/sessionstore/test/browser_crashedTabs.js#16-21

::: toolkit/components/printing/tests/browser_preview_switch_print_selected.js:14
(Diff revision 3)
> +add_task(function* set_simplify_pref() {
> +  // Ensure we have the simplify page preference set
> +	Services.prefs.setBoolPref("print.use_simplify_page", true);
> +});
> +
> + add_task(function* switch_print_preview_browsers() {

This is a really great test! Thanks for adding it!

::: toolkit/components/printing/tests/browser_preview_switch_print_selected.js:19
(Diff revision 3)
> + add_task(function* switch_print_preview_browsers() {
> +  let url = TEST_PATH + "simplifyArticleSample.html";
> +
> +  // Can only do something if we have a print preview UI:
> +  if (AppConstants.platform != "win" && AppConstants.platform != "linux") {
> +    ok(true, "Can't test if there's no print preview.");

We should fail here - in fact, this test should not be running, indicating a failure in our configuration. Let's make this throw by doing `ok(false, "Can't test...`

::: toolkit/components/printing/tests/browser_preview_switch_print_selected.js:45
(Diff revision 3)
> +  printPreviewToolbar.mSimplifyPageCheckbox.checked = true;
> +  printPreviewToolbar.simplify();

Alternatively, I think you could click on the checkbox (and assert it's then checked) with `printPreviewToolbar.mSimplifyPageCheckbox.click();`.

::: toolkit/components/printing/tests/browser_preview_switch_print_selected.js:64
(Diff revision 3)
> +  printPreviewToolbar.mSimplifyPageCheckbox.checked = false;
> +  printPreviewToolbar.simplify();

Same as above, with the click().

Comment 37

[Matheus Longaray (:mlongaray)]

(In reply to Mike Conley (:mconley) - Review / needinfo queue at max. from comment #35)
> What happens if you were to drop this into a .eslintrc.js file in
> toolkit/components/printing/test:
> 
> 
> "use strict";
> 
> module.exports = {
>   "extends": [
>     "plugin:mozilla/browser-test"
>   ]
> };
> 
> 
> Does that help? Are you able to make ESLint fail locally? Use ./mach eslint
> --setup to get it set up, and then run it against your test with:
> 
> ./mach eslint <path to test>

Yes, it did the work! Thanks Mike!

Comment 38

[Matheus Longaray (:mlongaray)]

Comment on attachment 8846804 [details]
Bug 1332386 - Create extra print preview browser when using Simplify Page option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119812/diff/3-4/

Comment 39

[Matheus Longaray (:mlongaray)]

Comment on attachment 8846804 [details]
Bug 1332386 - Create extra print preview browser when using Simplify Page option.

https://reviewboard.mozilla.org/r/119812/#review131524

> Alternatively, I think you could click on the checkbox (and assert it's then checked) with `printPreviewToolbar.mSimplifyPageCheckbox.click();`.

Oh, so much better! Why didn't I think about this first? _facepalm_

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8846804 [details]
Bug 1332386 - Create extra print preview browser when using Simplify Page option.

https://reviewboard.mozilla.org/r/119812/#review132676

So sorry for the days of delay here, mlongaray. :( I've gone through this, and it seems like it'll do the job. If you address the last two nits, I'll autoland it.

::: toolkit/components/printing/content/printUtils.js:622
(Diff revision 4)
>        gFocusedElement = document.commandDispatcher.focusedElement;
>  
> +      let changingPrintPreviewBrowsers = message.data.changingBrowsers;
>        let printPreviewTB = document.getElementById("print-preview-toolbar");
>        if (printPreviewTB) {
> +        if (changingPrintPreviewBrowsers) {

Nit - you can probably just check message.data.changingBrowsers right in here. I don't think you need the changingPrintPreviewBrowsers variable anymore.

::: toolkit/components/printing/tests/browser_preview_switch_print_selected.js:18
(Diff revision 4)
> +      ["print.use_simplify_page", true]
> +    ]
> +  });
> +});
> +
> + add_task(function* switch_print_preview_browsers() {

Nit: indentation

Comment 41

[Matheus Longaray (:mlongaray)]

Comment on attachment 8846804 [details]
Bug 1332386 - Create extra print preview browser when using Simplify Page option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119812/diff/4-5/

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6404057679f4
Create extra print preview browser when using Simplify Page option. r=mconley

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This seems to have caused frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=92468676&repo=autoland

Backed out in https://hg.mozilla.org/integration/autoland/rev/6c39975a195f

Comment 44

[Matheus Longaray (:mlongaray)]

Frequent failures indeed. Thanks for tracking that down, Wes.

So, I went looking for a good amount of failure examples hoping I could catch something. And I think I did.

All test failures pass first assertion at https://hg.mozilla.org/integration/autoland/file/6404057679f4/toolkit/components/printing/tests/browser_preview_switch_print_selected.js#l40 and time out after that.

See this screenshot sample: https://public-artifacts.taskcluster.net/c9qiAVFiT3GidLdRYmgWlQ/0/public/test_info//mozilla-test-fail-screenshot_4oVCjd.png (from https://treeherder.mozilla.org/logviewer.html#?job_id=92473041&repo=autoland&lineNumber=4319)

Given screenshot samples and where tests are timing out, I believe this happens because simplify page checkbox is somehow disabled, not being able to proceed with the assertions (we enable the option at https://hg.mozilla.org/integration/autoland/file/6404057679f4/toolkit/components/printing/tests/browser_preview_switch_print_selected.js#l13)
Weird thing is that the test run and pass normally sometimes, as it did in our push commit.

Ps: noticed that tests are not only timing out on Linux, but on Windows as well.

What do you think, Mike? Is there something we missed?

Comment 45

[Mike Conley (:mconley) (:⚙️)]

(In reply to Matheus Longaray (:mlongaray) from comment #44)
> 
> What do you think, Mike? Is there something we missed?

That's a good hypothesis. Perhaps use:

yield BrowserTestUtils.waitForCondition(() => {
  return printPreviewToolbar.mSimplifyPageCheckbox.enabled;
});

to hold the test until the checkbox is ready. Does that help address the issue?

Comment 46

[Matheus Longaray (:mlongaray)]

Okay, I'll try that. I'll push it for a try run and re-trigger particular mochitests to check if some of them still fail (hoping tests do not time out).

Comment 47

[Matheus Longaray (:mlongaray)]

Oh, maybe Reader Mode did not set source browser property isArticle in time or it did not detected/classified source browser as an article, so printUtils.js can use that property to enable simplify page checkbox.

Setting the preference as I said on comment 44 just sets mSimplifyPageCheckbox.hidden property to false.

I'll add more content to simplifyArticleSample.html mock as well as wait for condition be satisfied (comment 45).

Comment 48

[Matheus Longaray (:mlongaray)]

Try run on the way: https://treeherder.mozilla.org/#/jobs?repo=try&revision=acae2fc765631099ace4dccec612e8c24f1285fb&group_state=expanded

Added more content to simplifyArticleSample.html mock;

And we're now waiting for two conditions to be satisfied: source browser's property isArticle be set and simplify page checkbox enablement.

I'll re-trigger jobs containing our new test as soon as this try run is done.

Comment 49

[Matheus Longaray (:mlongaray)]

No success on last try run. Pushed yet another try run that sets reader.parse-on-load.enabled pref when running test (based on reader mode test cases). Let's see what happens.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33871a0b5b2e21dd51fb6ca929d6184ac1658218&group_state=expanded

Comment 50

[Matheus Longaray (:mlongaray)]

We're probably all good now after last try run. Setting up reader mode pref did the work (same way reader mode tests do). I re-triggered 10 times more those mochitests running browser_preview_switch_print_selected.js. No failures there.

I'll fold new commits, re-open last review and submit the adjusted test as soon as I have the time today.

Comment 51

[Matheus Longaray (:mlongaray)]

Comment on attachment 8846804 [details]
Bug 1332386 - Create extra print preview browser when using Simplify Page option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119812/diff/5-6/

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99fd6973d18e
Create extra print preview browser when using Simplify Page option. r=mconley

Comment 53

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/99fd6973d18e

Comment 54

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out because after this bug landed, we saw a spike of failures in browser_tabSwitchPrintPreview.js on Linux opt-ish builds, see bug 1308820:

https://hg.mozilla.org/mozilla-central/rev/21640269841ace0b6ab6818f89994558ef984b7a

List with failures: https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1308820
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=95337650&repo=autoland&lineNumber=2116
The screenshot taken after the failure occurred shows the print preview generation popup.

[task 2017-04-28T22:13:55.405773Z] 22:13:55     INFO - TEST-START | browser/base/content/test/tabs/browser_tabSwitchPrintPreview.js
[task 2017-04-28T22:14:00.217822Z] 22:14:00     INFO - GECKO(1532) | MEMORY STAT | vsize 20973843MB | residentFast 578MB
[task 2017-04-28T22:14:00.225923Z] 22:14:00     INFO - TEST-OK | browser/base/content/test/tabs/browser_tabSwitchPrintPreview.js | took 4822ms
[task 2017-04-28T22:14:00.403882Z] 22:14:00     INFO - checking window state
[task 2017-04-28T22:14:00.407629Z] 22:14:00     INFO - TEST-INFO | started process screentopng
[task 2017-04-28T22:14:02.587144Z] 22:14:02     INFO - TEST-INFO | screentopng: exit 0
[task 2017-04-28T22:14:02.590289Z] 22:14:02     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/tabs/browser_tabSwitchPrintPreview.js | Found an unexpected  at the end of test run - 
[task 2017-04-28T22:14:02.590623Z] 22:14:02     INFO - GECKO(1532) | must wait for focus

Comment 55

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Mike, the backout didn't fix the issue: https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1308820

Any idea what could trigger this?

Comment 56

[Mike Conley (:mconley) (:⚙️)]

Here's a screenshot from one of the failures:

https://public-artifacts.taskcluster.net/f8xUgVJVSjKAbRAFe7V85w/0/public/test_info//mozilla-test-fail-screenshot_aUbp81.png

It looks like we need to modify browser_tabSwitchPrintPreview.js to wait for the progress dialog to go away before it finishes the test.

Bug 1336763 might actually have caused this, since we're no longer spinning the event loop as often when closing tabs, so a race that the progress dialog used to win, it's probably losing now.

I'll write a patch to get this test to wait for the dialog to go away in bug 1308820. In the meantime, is it possible to re-land the patch in this bug?

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8528bf8f2ecf
Create extra print preview browser when using Simplify Page option. r=mconley

Comment 58

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8528bf8f2ecf

Comment 59

[Liz Henry (:lizzard) (relman/hg->git project)]

I'm marking this fix optional for 54.  You can still request uplift to 54, though.

Mike, what do you think? Too risky for 54 beta 11/12?

Comment 60

[Mike Conley (:mconley) (:⚙️)]

I don't think we need this for 54 - the Simplify Page feature is not set to ride out to release on 54 anyhow.

Comment 61

[Matheus Longaray (:mlongaray)]

Clearing ni? as per comment#60