Closed Bug 1180495 Opened 9 years ago Closed 9 years ago

[e10s] Clearing history does not clear the history for each tab.

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
e10s m8+ ---
firefox43 --- fixed

People

(Reporter: ke5trel, Assigned: Felipe)

References

Details

Attachments

(1 file, 3 obsolete files)

purge-history notification should properly clear SH on remote browsers
(deleted), patch
mconley
: review+
Details | Diff | Splinter Review 
Steps to reproduce:

1. Using an e10s window, navigate through a few pages to create a tab history.
2. Clear Recent History -> Browsing & Download History -> Everything
3. Note that the back button will turn gray indicating there is no history for this tab.
4. Switch to a different tab and then back to the previous tab.
5. The back button is no longer gray and shows previously visited pages.

Clearing history should clear each tab's history like with non-e10s windows.
Blocks: e10s
Status: UNCONFIRMED → NEW
tracking-e10s: --- → ?
Ever confirmed: true
Component: Bookmarks & History → Document Navigation
Product: Firefox → Core
Assignee: nobody → felipc
tracking-e10s: ? → m8+
Comment on attachment 8642859 [details] [diff] [review]
purge-history notification should properly clear SH on remote browsers

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

Fix looks good! I just have some suggestions about the test and one question regarding the setting of canGoForward / canGoBack.

::: browser/base/content/test/general/browser_purgehistory_clears_sh.js
@@ +3,5 @@
> +
> +const url = "http://example.org/browser/browser/base/content/test/general/dummy_page.html";
> +
> +add_task(function* purgeHistoryTest() {
> +  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, url);

I would suggest using BrowserTestUtils.withNewTab instead, as this will do the job of opening a new tab, and closing it properly / waiting for all of the right things on close.

Usage:

yield BrowserTestUtils.withNewTab({
  gBrowser,
  url,
}, function*(browser) {
  // Do things with browser
});

@@ +38,5 @@
> +    return content.history.length;
> +  });
> +
> +  is(historyAfterPurge, 1, "SHistory correctly cleared");
> +  is(gBrowser.selectedBrowser.webNavigation.canGoBack, false,

Alternatively:

ok(!gBrowser.selectedBrowser.webNavigation.canGoBack,
   "webNavigation.canGoBack correctly cleared");

Might be a good idea to have the user be in a state where they canGoForward and canGoBack before clearing the history, and then ensuring that canGoForward and canGoBack are set to false.

::: toolkit/content/widgets/remote-browser.xml
@@ +392,5 @@
> +      <method name="purgeSessionHistory">
> +        <body>
> +          <![CDATA[
> +            this.messageManager.sendAsyncMessage("Browser:PurgeSessionHistory");
> +            this.webNavigation.canGoBack = false;

I guess it's expected that remote-browser can reach in and modify these things?

If not, I wonder if RemoteWebProgress should wait for a message from the content process, and then set these, to keep webNavigation modification in the same place... what do you think?
Attachment #8642859 - Flags: review?(mconley) → review-
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #2)
> Comment on attachment 8642859 [details] [diff] [review]
Did the test changes as suggested, using history.back() to also test canGoForward. And also added checks for the disabled state of the UI's Back/Fwd buttons explained below.

> ::: toolkit/content/widgets/remote-browser.xml
> @@ +392,5 @@
> > +      <method name="purgeSessionHistory">
> > +        <body>
> > +          <![CDATA[
> > +            this.messageManager.sendAsyncMessage("Browser:PurgeSessionHistory");
> > +            this.webNavigation.canGoBack = false;
> 
> I guess it's expected that remote-browser can reach in and modify these
> things?
> 
> If not, I wonder if RemoteWebProgress should wait for a message from the
> content process, and then set these, to keep webNavigation modification in
> the same place... what do you think?

Yeah, I view RemoteWebNavigation/Progress as internal apis of remote-browser.xml, just separated in .jsms to reduce the xbl-ness of remote-browser. Besides, there's no message that we could listen for, so we would need to introduce a new one just for this. So I prefer to avoid the roundtrip here.
You can see that there's also precedent for this manual setting because the UI also does that, here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=ec8ea44bdb05#459

I figured there are probably no tests for this, so I added checks for this UI behavior too.
Attachment #8642859 - Attachment is obsolete: true
Attachment #8643131 - Flags: review?(mconley)
Comment on attachment 8643131 [details] [diff] [review]
purge-history notification should properly clear SH on remote browsers

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

Looks good - thanks felipe!
Attachment #8643131 - Flags: review?(mconley) → review+
Ah. We might need to account for the case where a browser has been removed from the DOM, and therefore has no frame loader?
Actually the problem was that the destructor needed to be updated like the constructor, otherwise it wouldn't remove the observer.

b-c green
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ed01762ab85
Attachment #8643131 - Attachment is obsolete: true
Flags: needinfo?(felipc)
Attachment #8643476 - Flags: review?(mconley)
Comment on attachment 8643476 [details] [diff] [review]
purge-history notification should properly clear SH on remote browsers

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

Good catch!
Attachment #8643476 - Flags: review?(mconley) → review+
Alright, this new version worked on try. As we decided on IRC, I kept the check of `if (this.docShell..)` in browser.xml in order to make it only add/remove the observer for non-remote browsers, and then let remote-browser.xml add/remove the observer by itself.
Attachment #8643476 - Attachment is obsolete: true
Attachment #8644385 - Flags: review?(mconley)
Comment on attachment 8644385 [details] [diff] [review]
purge-history notification should properly clear SH on remote browsers

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

Third time's the charm!

::: toolkit/content/widgets/browser.xml
@@ +873,5 @@
>        <!-- This is necessary because the destructor doesn't always get called when
> +           we are removed from a tabbrowser. This will be explicitly called by tabbrowser.
> +
> +           Note: this function is overriden in remote-browser.xml, so any clean-up that
> +           also applies to browser.isRemoteBrowser = true must be duplicated there. -->

This seems like a memory-leak footgun.

We might want to do the trick of having some _commonDestroyFunction in browser.xml that both destroys call into to do the common clean-up. Happy to have that filed as a follow-up though.

::: toolkit/content/widgets/remote-browser.xml
@@ +280,5 @@
>  
>        <!-- This is necessary because the destructor doesn't always get called when
> +           we are removed from a tabbrowser. This will be explicitly called by tabbrowser.
> +
> +           Note: This overrides the detroy() method from browser.xml. -->

Nit - "detroy" -> "destroy".
Attachment #8644385 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/098123688ca3
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: