1306294 - Restarting browser while Simplify Page mode on, restores 2 new empty tabs

Status: VERIFIED FIXED

(Toolkit :: Printing, defect, P3)

Description

[Bogdan Maris, Desktop Test Engineering]

[Affected versions]:
- Latest Nightly 52.0a1 (2016-09-28)
- Latest Aurora 51.0a2 (2016-09-29)
- Firefox 50.0b2

[Affected platforms]:
- Windows 10 Pro x64
- Windows 7 x64
- Ubuntu 16.04 LTS x64 / x32

[Steps to reproduce]:
1. Start Firefox
2. Visit a page with Reader View (ex: wikipedia.org)
3. Go to File -> Print Preview
4. Click on "Simplify Page" checkbox
5. Restart browser (Shift + F2)

[Expected result]:
- Previous session is restored properly.

[Actual result]:
- “Simplify Page” mode is off and two new empty tabs are restored (one of them has the title of the tab where the “Simplify Page” mode was on).

[Regression range]:
- This is not a regression since the behavior reproduces on Nightly build (2016-06-06), when it first landed.

[Additional notes]:
- When clicking the new empty tab which has the title of the tab where the “Simplify Page” mode was on, that title is no more displayed.
- Only one new empty tab will be displayed when restarting from Print Preview.

Comment 1

[Bogdan Maris, Desktop Test Engineering]

And here is a screencast with the issue: https://goo.gl/0y7XGw (was to large to upload it directly to bugzilla).

Comment 2

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

We shouldn't be persisting these tabs.

We should be returning false here: http://searchfox.org/mozilla-central/rev/fd672b97f13aa83af5f04caa7b61bd443fb623e9/browser/components/sessionstore/SessionStore.jsm#3892

Which should make sure that the tab is not persisted to the session store. mlongaray, if you have a second, do you think you could see what's going on here? Are we returning true for this SessionStore method when we should be returning false? If so, why?

Comment 3

[Matheus Longaray (:mlongaray)]

Hey Mike, I managed to debug SessionStore.jsm and could confirm the following.

This method (http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#3892) does return false indeed when print-preview-related-tabs are closed. However, this method is only called when there is a 'close' event. If the browser closes abruptly, this method does not get called.

When the browser starts up again, it will end up on this piece of code (http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#1192) in which inits a Session with, what I presumed to be, all windows/tabs that were open prior crashing.

initSession() checks if the previous session crashed, so it can show the about:sessionrestore page (http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#548).

What do you recommend for us to do? Should we remove unwanted tabs from the initial state? (http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#523)

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I was able to repro this and I think we should consider fixing this in 51 and perhaps uplifting to 50 if the fix is simple enough. Not sure if P3 is appropriate here, seems more like a P2 to me i.e. fix in the next cycle like 51.

Comment 5

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

Hey mlongaray - you pinged me in IRC saying that you had a patch. Can I see it?

Comment 6

[Matheus Longaray (:mlongaray)]

Attachment: WIP1: Remove not worth-saving tabs when restoring a crashed session

When recovering from a crash, we restore all windows and tabs from last session. This patch filters out not worth-saving tabs, i.e., tabs that only have a transient about: entry, no other session history, and no userTypedValue.

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

We've made a decision to turn off "Simplify Page" feature on Fx50. This is now a wontfix for that version.

Comment 8

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

Comment on attachment 8800792 [details] [diff] [review]
WIP1: Remove not worth-saving tabs when restoring a crashed session

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

Hm. This kinda seems like wallpapering over a problem. Why are we even storing the state if the state would be uninteresting if the tab or window closed?

Hey mikedeboer, do you know of a good reason why we only compute _shouldSaveTabState on window or tab close, and not when we're otherwise getting tab or window state?

Like, in _collectWindowData, we go ahead and collect each tab's state, regardless of _shouldSaveTabState... should we perhaps do a _shouldSaveTabState there as well? I think that'd be preferable over filtering out things that we don't want to persist after having already reloaded it off the disk.

Comment 9

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

ni? to mikedeboer for comment 8. It's cool if you're not sure - I know _shouldSaveTabState is from before you were module owner. We might be able to redirect to ttaubert, but thought I'd check with you first.

Comment 10

[Mike de Boer [:mikedeboer]]

Yes, I'd say that `_shouldSaveTabState` can/ should be used in `_collectWindowData` too, but I'd like to learn from Tim why that's not the case currently.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

I think this is mainly due to the semantic difference between collecting window data and saving it. The function is called `_shouldSaveTabState()` because it tells you whether we deem that tab's data worth saving, although of course we also use it to determine whether it will show up in the list of closed tabs for a window, or a window will go into the list of closed windows.

`SessionStore.getBrowserState()` or `.getWindowState()` returns the in-memory state, and doesn't filter tabs. Whether that's important I don't know, but there of course might be add-ons that depend on this. There won't be WebExtensions depending on this though, but maybe there are other consumers inside Firefox.

So either you can change `_collectWindowData()`, or you can filter with `_shouldSaveTabState()` again in `SessionSaver._saveState()`.

Comment 12

[Matheus Longaray (:mlongaray)]

Attachment: WIP2: Move filtering to SessionSaver._saveState()

This patch moves filtering of not worth-saving tabs to SessionSaver._saveState()

Comment 13

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

Comment on attachment 8803865 [details] [diff] [review]
WIP2: Move filtering to SessionSaver._saveState()

The general idea looks okay to me. I think you'll want mikedeboer to review it though.

Thanks!

Comment 14

[Matheus Longaray (:mlongaray)]

Comment on attachment 8803865 [details] [diff] [review]
WIP2: Move filtering to SessionSaver._saveState()

Asking for review from mikedeboer after mconley granted feedback.

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8803865 [details] [diff] [review]
WIP2: Move filtering to SessionSaver._saveState()

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

I'm sorry, but if it's at all possible, I'd like the change to happen in `_collectWindowData()`.
Essentially, from this:
```js
// update the internal state data for this window
for (let tab of tabs) {
  let tabData = TabState.collect(tab);
  tabMap.set(tab, tabData);
  tabsData.push(tabData);
}
```

to:

```js
// Update the internal state data for this window.
for (let tab of tabs) {
  let tabData = TabState.collect(tab);
  if (!this._shouldSaveTabState(tabData))
    continue;
  tabMap.set(tab, tabData);
  tabsData.push(tabData);
}
```

This might seem scary, but hey, those are the changes most fun to work on!
When you push this change to try - after you've confirmed that it fixes this bug too - you'll find out quickly enough whether works across the board.

Comment 16

[Mike de Boer [:mikedeboer]]

BTW, I'm here to help - you can find me on IRC in #developers or #fx-team to discuss things!

Comment 17

[Matheus Longaray (:mlongaray)]

Attachment: WIP3: Move filtering to _collectWindowData()

This patch moves filtering of not worth-saving tabs to SessionStoreInternal._collectWindowData()

Comment 18

[Mike de Boer [:mikedeboer]]

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

Comment 19

[Mike de Boer [:mikedeboer]]

According to that try run we have work to do! So what I'll do asap is analyze the results and see if we can fix the tests that are failing right now. 'browser_urlbarAboutHomeLoading.js' is making me a bit worried, because that's a behavior regression test.
I think that's something you'll want me to do, am I right?
There are two scenarios:
1) I can find simple fixes for all the tests
2) I can't and we can use your previous patch that'll filter the state dictionary.

Comment 20

[Mike de Boer [:mikedeboer]]

Comment on attachment 8805535 [details] [diff] [review]
WIP3: Move filtering to _collectWindowData()

Please re-request review when you've got a new patch ready for me to look at. Thanks!

Comment 21

[Matheus Longaray (:mlongaray)]

Attachment: WIP4: Move filtering to SessionSaver._saveState()

As discussed on IRC with mikedeboer, we have decided to go back to this approach given its a lot less invasive. This patch moves filtering of not worth-saving tabs to SessionSaver._saveState().

Comment 22

[Mike de Boer [:mikedeboer]]

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

Comment 23

[Matheus Longaray (:mlongaray)]

Attachment: WIP5: Make selectedWindow attribute concise and adjust session restore tests

This patch addresses concerns raised about selectedWindow and selected attributes if we happen to remove a not-worthy tab that is currently selected. Since we will get rid of not-worthy tabs from our session state, this patch also adjusts a few tests as needed.

Comment 24

[Mike de Boer [:mikedeboer]]

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

Comment 25

[Mike de Boer [:mikedeboer]]

Comment on attachment 8808591 [details] [diff] [review]
WIP5: Make selectedWindow attribute concise and adjust session restore tests

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

Pending a green try run, r=me!

::: browser/components/sessionstore/SessionSaver.jsm
@@ +185,5 @@
>      stopWatchStart("COLLECT_DATA_MS", "COLLECT_DATA_LONGEST_OP_MS");
>      let state = SessionStore.getCurrentState(forceUpdateAllWindows);
>      PrivacyFilter.filterPrivateWindowsAndTabs(state);
>  
> +    // Make sure we only write worth saving tabs to disk

nit: missing dot.

Comment 26

[Matheus Longaray (:mlongaray)]

Attachment: WIP6: Address nit

This patch addresses missing dot.

Comment 27

[Matheus Longaray (:mlongaray)]

Status:

Last try run is showing that browser/components/sessionstore/test/browser_819510_perwindowpb.js is failing on Linux. However, this test is passing when I run it locally and it does makes sense to pass. This same test fails on the other platforms.

browser/components/sessionstore/test/browser_newtab_userTypedValue.js is failing on OS X, and when I run it locally on Linux, sometimes it works as expected, sometimes it does not (after different builds without any code change on the test). Maybe a timing issue?

Comment 28

[Matheus Longaray (:mlongaray)]

Attachment: WIP7: Fix browser_819510_perwindowpb.js

This patch fixes an issue with browser_819510_perwindowpb.js test.

Comment 29

[Mike de Boer [:mikedeboer]]

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

Comment 30

[Mike de Boer [:mikedeboer]]

Comment on attachment 8809930 [details] [diff] [review]
WIP7: Fix browser_819510_perwindowpb.js

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

Matheus, doesn't seem like the changes you made are quite enough yet! :(
On OSX, we've got a failing browser/components/sessionstore/test/browser_scrollPositions.js (non-e10s) and a failing browser/components/sessionstore/test/browser_newtab_userTypedValue.js (e10s).
On Windows we've got a failing browser/components/sessionstore/test/browser_scrollPositionsReaderMode.js and browser/components/sessionstore/test/browser_scrollPositions.js (non-e10s) and a failing browser/components/sessionstore/test/browser_newtab_userTypedValue.js (e10s).

I'm r- the patch just because of this, so once you've got everything green, this'll automatically turn into an r+.

Comment 31

[Matheus Longaray (:mlongaray)]

Attachment: WIP8: Ensure only primary window is opened after browser_625016.js is done

Comment 32

[Mike de Boer [:mikedeboer]]

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

Comment 33

[Matheus Longaray (:mlongaray)]

Attachment: WIP9: Revert to WIP6 fixing only browser_819510_perwindowpb.js

Comment 34

[Mike de Boer [:mikedeboer]]

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

Comment 35

[Matheus Longaray (:mlongaray)]

Attachment: WIP10: Fix browser_scrollPositions.js and browser_scrollPositionsReaderMode.js test

Following is the progress for WIP10 build on Treeherder.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57cdf373a4939647b997749f8c9549665bf9c9cc&selectedJob=31469395

If you have the time, could you take a look at these results - Mike? I believe we would only have browser_newtab_userTypedValue.js test pending to fix.

Comment 36

[Mike de Boer [:mikedeboer]]

These results are looking good to, but I don't understand why browser_newtab_userTypedValue.js is failing, because it's passing just fine for me locally (OSX 10.12 debug build), so I'm not sure what's up there...
With that try push I'm also not seeing the commit(s) you pushed to try, so I'm curious what you were running there.

Comment 37

[Matheus Longaray (:mlongaray)]

(In reply to Mike de Boer [:mikedeboer] from comment #36)
> These results are looking good to, but I don't understand why
> browser_newtab_userTypedValue.js is failing, because it's passing just fine
> for me locally (OSX 10.12 debug build), so I'm not sure what's up there...

Yes, same for me. browser_newtab_userTypedValue.js is failing locally for me but passing on try.

> With that try push I'm also not seeing the commit(s) you pushed to try, so
> I'm curious what you were running there.

Here's what I pushed: https://hg.mozilla.org/try/rev/be22a9cfa9af

Comment 38

[Matheus Longaray (:mlongaray)]

Attachment: WIP11: Fix browser_newtab_userTypedValue.js

Following is the progress for WIP11 build on Treeherder.

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

Comment 39

[Matheus Longaray (:mlongaray)]

Comment on attachment 8812845 [details] [diff] [review]
WIP11: Fix browser_newtab_userTypedValue.js

Bug 1306294 - Filters out not worth-saving tabs from session state. r=mikedeboer

Comment 40

[Mike de Boer [:mikedeboer]]

Comment on attachment 8812845 [details] [diff] [review]
WIP11: Fix browser_newtab_userTypedValue.js

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

::: browser/components/sessionstore/test/browser_scrollPositions.js
@@ +112,5 @@
>  add_task(function test_scroll_background_tabs() {
>    pushPrefs(["browser.sessionstore.restore_on_demand", true]);
>  
>    let newWin = yield BrowserTestUtils.openNewBrowserWindow();
> +  BrowserTestUtils.loadURI(newWin.gBrowser, "http://www.example.com/");

Why did you put this change here and why did it help?
(This is not correct, because you're passing in `gBrowser` and not a xul:browser instance that's attached to a tab.)

::: browser/components/sessionstore/test/browser_scrollPositionsReaderMode.js
@@ +20,5 @@
>  add_task(function test_scroll_background_about_reader_tabs() {
>    pushPrefs(["browser.sessionstore.restore_on_demand", true]);
>  
>    let newWin = yield BrowserTestUtils.openNewBrowserWindow();
> +  BrowserTestUtils.loadURI(newWin.gBrowser, "http://www.example.com/");

Same here.

Comment 41

[Matheus Longaray (:mlongaray)]

Attachment: WIP12: Change browser_scrollPositions.js and browser_scrollPositionsReaderMode.js test

Bug 1306294 - Filters out not worth-saving tabs from session state. r=mikedeboer

Following is the progress for WIP12 build on Treeherder.

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

Comment 42

[Mike de Boer [:mikedeboer]]

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

Comment 43

[Mike de Boer [:mikedeboer]]

Comment on attachment 8813186 [details] [diff] [review]
WIP12: Change browser_scrollPositions.js and browser_scrollPositionsReaderMode.js test

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

r=me! \o/ Great job, Matheus, persevering and pulling this through the end!
I hope it'll stick when it lands. Fingers crossed.

I'm looking forward to seeing more of your work!

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b75d4672954
"Restarting browser while Simplify Page mode on, restores 2 new empty tabs". r=mdeboer

Comment 45

[Carsten Book [:Tomcat]]

sorry had to back this out for new test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=39706918&repo=mozilla-inbound

Comment 46

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4772a0f4bed2
Backed out changeset 4b75d4672954 for bc5 test failures

Comment 47

[Matheus Longaray (:mlongaray)]

(In reply to Carsten Book [:Tomcat] from comment #45)
> sorry had to back this out for new test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=39706918&repo=mozilla-
> inbound

No worries! Let's see if we can find out what's going on there. Thanks for letting us know.

Comment 48

[Matheus Longaray (:mlongaray)]

Attachment: WIP13: Fix browser_newtab_userTypedValue.js and browser_urlbarAboutHomeLoading.js

Commit message: Bug 1306294 - Filters out not worth-saving tabs from session state. r=mikedeboer

Our last patch set had to be backed out due some bc test failures (browser_newtab_userTypedValue.js & browser_urlbarAboutHomeLoading.js)

Following is the progress for WIP13 build on Treeherder. I re-triggered the respective bc test suites 10 times more for all platforms. No failures this time.

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

Comment 49

[Matheus Longaray (:mlongaray)]

Comment on attachment 8816973 [details] [diff] [review]
WIP13: Fix browser_newtab_userTypedValue.js and browser_urlbarAboutHomeLoading.js

Commit message: Bug 1306294 - Filters out not worth-saving tabs from session state. r=mikedeboer

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe1b40baf57
Filters out not worth-saving tabs from session state. r=mikedeboer

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cbe1b40baf57

Comment 52

[Matheus Longaray (:mlongaray)]

Attachment: WIP14: Add about:printpreview redirector

As discussed on bug 1323987, we are not allowed to remove "about:blank" and "about:newtab" tabs from our session state when saving it to disk - these tabs should be persisted.

Since preview opens up "about:blank" tab in background to put the document into print preview mode, we are adding a new entry in our about: redirector called "about:printpreview". In this way, when we are print previewing and session store kicks in to save our opened tabs to disk, session store will discard this kind of tab - not saving it to disk.

This patch updates our browser code to use about:printpreview and makes expected changes to SessionStore. It also updates the internal notion of nsDocShell to manually set our document's URI when creating a blank document for print preview content viewer.

Comment 53

[Matheus Longaray (:mlongaray)]

Comment on attachment 8821283 [details] [diff] [review]
WIP14: Add about:printpreview redirector

On -175,17 +180,19 @@ AboutRedirector::NewChannel(nsIURI* aURI:

I tested URI_NORELATIVE flag (protocol doesn't support relative URIs, e.g., about and javascript) instead and it happens to work in our case. What I'm not sure is that if we are going to be causing repercussions using this flag - other than about:printpreview. See more at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIProtocolHandler.

Comment 54

[Mike de Boer [:mikedeboer]]

Comment on attachment 8821283 [details] [diff] [review]
WIP14: Add about:printpreview redirector

Matheus,

Overall this patch looks like it's going in the right direction, but it doesn't belong to this bug.
Can you attach it to bug 1323987 instead?
Can you also put a treeherder link to the try job you pushed or are about to push?

Thanks!

Comment 55

[Matheus Longaray (:mlongaray)]

Sorry, Mike - my mistake. I'll move this patch to bug 1323987 as well as post the treeherder link.

Comment 56

[Gerry Chang [:gchang]]

We've built 51 RC and it's too late for 51. Mark 51 won't fix.

Comment 57

[Sylvestre Ledru [:Sylvestre]]

In the release for several versions, we should just let it ride the train.

Comment 58

[Alice0775 White]

Bug 1342849 is caused this.
Could you back out this from Aurora53.0a2 before merge to beta.

Comment 59

[u589863]

Bug 1343056 is also caused by this.
Could you back out

Comment 60

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Mike, it seems like there are several issues (bug 1342849, bug 1343056) with this, but it's not clear to me if anyone's watching this bug.

Who can decide if we need to take action here?

Comment 61

[Matheus Longaray (:mlongaray)]

Clearing out need info request - currently working on bug 1343056 and bug 1342849.

Comment 62

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #60)
> Mike, it seems like there are several issues (bug 1342849, bug 1343056) with
> this, but it's not clear to me if anyone's watching this bug.
> 
> Who can decide if we need to take action here?

Just an update here; we found out that bug 1342849 is regressed by a different bug. We suspect we can do a simple fix for bug 1343056, and mlongaray is working on that.

Comment 63

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this bug using Fx 52.0a1, build ID: 20160928030201, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 53.0b1, build ID:  20170307064827, on Windows 10 x64.

Cheers!