Closed Bug 1163745 Opened 5 years ago Closed 4 years ago

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

Categories

(Firefox :: Session Restore, defect)

26 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 41
Tracking Status
firefox38 --- wontfix
firefox39 + wontfix
firefox38.0.5 --- wontfix
firefox40 + wontfix
firefox41 + verified
firefox-esr31 --- wontfix
firefox-esr38 --- wontfix

People

(Reporter: alice0775, Assigned: ttaubert)

References

Details

(Keywords: regression, ux-consistency)

Attachments

(2 files, 1 obsolete file)

[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
Blocks: 1163700
No longer blocks: 163700
Adding a tracking flag for firefox40 and 41. Tim, can you please take a look at this as its related to bug 1162036?
Flags: needinfo?(ttaubert)
Flags: firefox-backlog?
Flags: qe-verify+
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.
Flags: needinfo?(ttaubert)
(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 :)
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
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
Attachment #8617894 - Flags: review?(dteller)
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
Attachment #8617897 - Flags: review?(dteller)
Attachment #8617894 - Flags: review?(dteller) → review+
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?
Attachment #8617897 - Flags: review?(dteller) → feedback+
Sorry about the review lag. Life is complicated and Yosemite has killed (or at least gravely wounded) my laptop.
(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 on attachment 8625558 [details] [diff] [review]
0002-Bug-1163745-Properly-support-shistory-purging-for-pe.patch, v2

Oops.
Attachment #8625558 - Flags: review?(mak77) → review?(dteller)
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
Attachment #8625558 - Flags: review?(dteller) → review+
https://hg.mozilla.org/mozilla-central/rev/f109e82854fe
https://hg.mozilla.org/mozilla-central/rev/5e6f90f35f9b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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.
(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.
Reproduced the initial issue on Firefox 40.0b9, build ID: 20150730171029.

Confirming the fix on latest 41.0a2, build ID: 20150802004005.
Status: RESOLVED → VERIFIED
QA Contact: cornel.ionce
I have the impression that purging shistory still isn't implemented in e10s. Am I wrong?
Flags: needinfo?(ttaubert)
Flags: needinfo?(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.
Flags: needinfo?(ttaubert)
Flags: needinfo?(mconley)
Flags: firefox-backlog?
You need to log in before you can comment on or make changes to this bug.