If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in Firefox 41

Status

()

Firefox
Session Restore
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Alice0775 White, Assigned: ttaubert)

Tracking

({regression, ux-consistency})

26 Branch
Firefox 41
regression, ux-consistency
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog ?
qe-verify +

Firefox Tracking Flags

(firefox38 wontfix, firefox38.0.5 wontfix, firefox39+ wontfix, firefox40+ wontfix, firefox41+ verified, firefox-esr31 wontfix, firefox-esr38 wontfix)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
[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
(Reporter)

Updated

2 years ago
status-firefox-esr31: unaffected → affected
(Reporter)

Updated

2 years ago
Blocks: 1163700
No longer blocks: 163700

Comment 1

2 years ago
Adding a tracking flag for firefox40 and 41. Tim, can you please take a look at this as its related to bug 1162036?
tracking-firefox39: ? → +
tracking-firefox40: ? → +
tracking-firefox41: --- → +
Flags: needinfo?(ttaubert)
Flags: firefox-backlog?

Updated

2 years ago
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)
status-firefox38.0.5: ? → affected
status-firefox39: affected → wontfix
(Assignee)

Comment 3

2 years ago
(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
(Assignee)

Comment 4

2 years ago
Created attachment 8617894 [details] [diff] [review]
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
Attachment #8617894 - Flags: review?(dteller)
(Assignee)

Comment 5

2 years ago
Created attachment 8617897 [details] [diff] [review]
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
Attachment #8617897 - Flags: review?(dteller)
Attachment #8617894 - Flags: review?(dteller) → review+

Comment 6

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/bcec28b46718
(Assignee)

Comment 7

2 years ago
(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 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.
(Assignee)

Comment 10

2 years ago
(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.
(Assignee)

Comment 11

2 years ago
Created attachment 8625558 [details] [diff] [review]
0002-Bug-1163745-Properly-support-shistory-purging-for-pe.patch, v2
Attachment #8617897 - Attachment is obsolete: true
Attachment #8625558 - Flags: review?(mak77)
(Assignee)

Comment 12

2 years ago
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+

Comment 14

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/f109e82854fe
https://hg.mozilla.org/integration/fx-team/rev/5e6f90f35f9b
https://hg.mozilla.org/mozilla-central/rev/f109e82854fe
https://hg.mozilla.org/mozilla-central/rev/5e6f90f35f9b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: ? → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
status-firefox38: affected → wontfix
status-firefox38.0.5: affected → wontfix
(Assignee)

Updated

2 years ago
status-firefox-esr31: affected → wontfix
status-firefox-esr38: ? → wontfix
(Assignee)

Comment 16

2 years ago
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.
status-firefox41: fixed → wontfix
(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.
status-firefox40: affected → wontfix
status-firefox41: wontfix → fixed
Reproduced the initial issue on Firefox 40.0b9, build ID: 20150730171029.

Confirming the fix on latest 41.0a2, build ID: 20150802004005.
Status: RESOLVED → VERIFIED
status-firefox41: fixed → 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)
You need to log in before you can comment on or make changes to this bug.