1163745 - Clicking a suspend tab should show the page content after doing "Clear Recent History..."

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: Regression

Steps To Reproduce:
0. Start Nightly with newly created profile
1. Open several tabs
2. Exit and restart the browser
3. Click "Restore Previous Session" from History Menu
4. Click "Clear Recent History..." from History Menu
5. Click [Clear Now] in the Clear All History dialog
6. Click background tab(suspend tab)

Actual Results:
Clicking a suspend tab does not show the page content.

Expected Results:
Clicking a suspend tab should show the page content


Note:
On Nightly40.0a1, Bug 1162036 is fixing wrong way, but not fixed this underlying bug.


Regression window:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=cead6eb63964&tochange=6ad5e7c6ecf2

Regressed by: Bug 903244

Comment 1

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

Adding a tracking flag for firefox40 and 41. Tim, can you please take a look at this as its related to bug 1162036?

Comment 2

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

Tim, are you able to have a look at this soon? It is too late to take this for 39, but we could still take a patch for 41 and 40.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Liz Henry (:lizzard) from comment #2)
> Tim, are you able to have a look at this soon? It is too late to take this
> for 39, but we could still take a patch for 41 and 40.

Taking. We might look into uplifting indeed, although it has been broken since 26 without anyone noticing :)

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1163745-Remove-erroneous-subtest-from-browser_se.patch

This patch removes a pointless subtest from browser_sessionHistory.js. When I wrote that I assumed that "browser:purge-session-history" would be dispatched in the child as well but that's nonsense in an e10s world.

e10s currently doesn't support purging a tab's shistory [1]. Once that's implemented though it will automatically work with the next patch I'll attach.

[1] http://hg.mozilla.org/mozilla-central/annotate/e10e2e8d8bf2/toolkit/content/widgets/browser.xml#l919

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1163745-Properly-support-shistory-purging-for-pe.patch

This patch removes the code that resets pending tabs and instead simply relies on the content script's SHistory listener. That will update the internal state whenever shistory entries are purged and the pending tab will restore with only the current shistory entry. Only that single entry will be saved to disk when quitting without restoring the pending tabs.

The OnHistoryNewEntry() handler had to be changed as the <browser> implementation [1] re-adds the current shistory entry and pushes it onto the end. This is safe to do for pending tabs as that doesn't signal fragment navigation.

[1] http://hg.mozilla.org/mozilla-central/annotate/e10e2e8d8bf2/toolkit/content/widgets/browser.xml#l924

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bcec28b46718

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Pulsebot from comment #6)
> https://hg.mozilla.org/integration/fx-team/rev/bcec28b46718

That's um... from bug 1173857. Chose the wrong bug number :( Backed out.

https://hg.mozilla.org/integration/fx-team/rev/c827dc4915ba

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8617897 [details] [diff] [review]
0002-Bug-1163745-Properly-support-shistory-purging-for-pe.patch

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

::: browser/components/sessionstore/ContentRestore.jsm
@@ +345,5 @@
>    // the given |uri| only differs in the fragment.
>    OnHistoryNewEntry(newURI) {
> +    let currentURI = this.webNavigation.currentURI;
> +
> +    // Ignore new SHistory entries with the same URI.

Can you document why?

::: browser/components/sessionstore/SessionStore.jsm
@@ +1272,3 @@
>      let openWindows = {};
> +    // Collect open windows.
> +    this._forEachBrowserWindow(({__SSi: id}) => openWindows[id] = true);

How many times has this line changed? :)

Why don't we need to reset the tab restoring state anymore?

::: browser/components/sessionstore/test/browser_purge_shistory.js
@@ +1,2 @@
> +"use strict";
> +

Can you explain what this test checks?

@@ +1,4 @@
> +"use strict";
> +
> +const TAB_STATE = {
> +  entries: [{url: "about:mozilla"}, {url: "about:robots"}],

To help tracking leaked windows, could you make this about:robots?test=browser_purge_shistory.js, or something like that?

@@ +36,5 @@
> +  yield promise;
> +
> +  // Purge session history.
> +  Services.obs.notifyObservers(null, "browser:purge-session-history", "");
> +  ok((yield checkTabContents(browser)), "expected tab contents found");

For sanity's sake, you could check that the tab is still pending.

@@ +41,5 @@
> +
> +  // Kick off tab restoration.
> +  gBrowser.selectedTab = tab2;
> +  yield promiseTabRestored(tab2);
> +  ok((yield checkTabContents(browser2)), "expected tab contents found");

Shouldn't you check that the tab is not pending anymore?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Sorry about the review lag. Life is complicated and Yosemite has killed (or at least gravely wounded) my laptop.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach-Teller [:Yoric] (away June 22 - July 7th, use "needinfo") from comment #8)
> > +    let currentURI = this.webNavigation.currentURI;
> > +
> > +    // Ignore new SHistory entries with the same URI.
> 
> Can you document why?

Done.

> > +    // Collect open windows.
> > +    this._forEachBrowserWindow(({__SSi: id}) => openWindows[id] = true);
> 
> How many times has this line changed? :)

Not sure, did I touch this before? Honestly don't know.

> Why don't we need to reset the tab restoring state anymore?

I don't even know why we did that before because we want the tab to still be pending. We leave it pending, and if the shistory is updated properly (which only works in e10s currently) then it will send updates and update the TabStateCache.

> ::: browser/components/sessionstore/test/browser_purge_shistory.js
> @@ +1,2 @@
> > +"use strict";
> > +
> 
> Can you explain what this test checks?

Done.

> > +const TAB_STATE = {
> > +  entries: [{url: "about:mozilla"}, {url: "about:robots"}],
> 
> To help tracking leaked windows, could you make this
> about:robots?test=browser_purge_shistory.js, or something like that?

This is weird, we don't do that anywhere else and the harness can usually figure out what test leaks.

> > +  // Purge session history.
> > +  Services.obs.notifyObservers(null, "browser:purge-session-history", "");
> > +  ok((yield checkTabContents(browser)), "expected tab contents found");
> 
> For sanity's sake, you could check that the tab is still pending.

Done.

> > +  // Kick off tab restoration.
> > +  gBrowser.selectedTab = tab2;
> > +  yield promiseTabRestored(tab2);
> > +  ok((yield checkTabContents(browser2)), "expected tab contents found");
> 
> Shouldn't you check that the tab is not pending anymore?

We check that stuff in a ton of other tests but it doesn't hurt I guess.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1163745-Properly-support-shistory-purging-for-pe.patch, v2

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8625558 [details] [diff] [review]
0002-Bug-1163745-Properly-support-shistory-purging-for-pe.patch, v2

Oops.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8625558 [details] [diff] [review]
0002-Bug-1163745-Properly-support-shistory-purging-for-pe.patch, v2

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

rubberstamp=me

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f109e82854fe
https://hg.mozilla.org/integration/fx-team/rev/5e6f90f35f9b

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f109e82854fe
https://hg.mozilla.org/mozilla-central/rev/5e6f90f35f9b

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

I'd rather let this ride the trains and check that we have no reports of this not working. This issue was around for quite some time so I think it's not super important to uplift anyway.

Comment 17

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Tim Taubert [:ttaubert] from comment #16)
> I'd rather let this ride the trains and check that we have no reports of
> this not working. This issue was around for quite some time so I think it's
> not super important to uplift anyway.

Looks like a small mistake in flags.

Comment 18

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced the initial issue on Firefox 40.0b9, build ID: 20150730171029.

Confirming the fix on latest 41.0a2, build ID: 20150802004005.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

I have the impression that purging shistory still isn't implemented in e10s. Am I wrong?

Comment 20

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

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #19)
> I have the impression that purging shistory still isn't implemented in e10s.
> Am I wrong?

Yes, we have that. This was done in bug 1180495.