464199 - Forget About this Site doesn't forget about recently closed tabs

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P1)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

STR:

1. Go to www.beltzner.ca/mike
2. Close the tab
3. History > Show all History
4. Find beltzner.ca and right-click
5. Select "Forget About this Site"
6. History > Recently Closed Tabs

Expected: no beltzner.ca/mike

Actual: beltzner.ca/mike is at the top of the list

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

(not sure if it should also close any *open* tabs; that would be a different bug)
(also not sure what to do about the sessionstore.js written on disk)

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

I personally think we should take out the open tabs, saying "forget everything" and still seeing the site sends the wrong message to the user, especially when they are carrying out the action from the site panel.

Comment 3

[Simon Bünzli]

Note to self: We'll also have to make sure to clear out the respective cookies.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #2)
> I personally think we should take out the open tabs, saying "forget everything"
> and still seeing the site sends the wrong message to the user, especially when
> they are carrying out the action from the site panel.

As I said, that would be another bug. I filed bug 464417 for what to do with tabs that are already open.

(In reply to comment #3)
> Note to self: We'll also have to make sure to clear out the respective cookies.

It already does that, I thought.

Comment 5

[Simon Bünzli]

Attachment: WIP

Comment 6

[Simon Bünzli]

Attachment: browser:purge-domain-data

This patch adds a new notification so that extensions and other components can easily clean domain specific data themselves. For cleaning out the closed tabs, a regex is the easiest solution since otherwise we'd have to check in quite a lot of places (recursing into subframe URIs, formdata of various forms, extension set tab values, etc.).

Comment 7

[Shawn Wilsher :sdwilsh]

Is there any reason why this code doesn't use the same logic the rest of the code uses to determine if we have a domain match?
http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#43

Comment 8

[Simon Bünzli]

(In reply to comment #7)
That code only works on hosts (it's mostly an .endswith implementation) which would be quite convoluted to get from all the possible places where we store URLs (see comment #6). It should behave pretty much the same, though. Doesn't it?

Comment 9

[Shawn Wilsher :sdwilsh]

Yeah - I think it behaves the same.  I just wanted to make sure we weren't doing things differently for the sake of doing them differently.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Gavin: can you expedite review here or reassign?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 354163 [details] [diff] [review]
browser:purge-domain-data

>diff -r 7127f726f661 browser/components/sessionstore/src/nsSessionStore.js

>+    case "browser:purge-domain-data":
>+      let domainRe = new RegExp("[/.]" + aData.replace(/\W/g, "\\$&") + "[:/]", "i");
>+      // remove all closed tabs containing a reference to the given domain
>+      for (let ix in this._windows) {
>+        let closedTabs = this._windows[ix]._closedTabs;
>+        for (let i = closedTabs.length - 1; i >= 0; i--) {
>+          if (domainRe.test(closedTabs[i].toSource()))
>+            closedTabs.splice(i, 1);
>+        }
>+      }

This concerns me a lot... I don't like the idea of relying on a regexp match on the toSource() serialization to find the right tabs. What happens if a form field in the tab happens to contain the hostname as a string, or something like that?

Are there really too many fields to iterate over directly? Ideally we'd create nsIURIs and do actual host compares as well... I don't think we need to care _too_ much about perf here.

Patch looks OK otherwise, but I'm not familiar enough with the sessionstore code to be able to tell offhand whether just splicing elements from _closedTabs is safe - I'm assuming you are :)

Comment 12

[Simon Bünzli]

Attachment: browser:purge-domain-data (without RegExp)

(In reply to comment #11)
> What happens if a form field in the tab happens to contain the hostname as a
> string, or something like that?

The tab would be removed. Then again...

> Are there really too many fields to iterate over directly?

I was originally concerned about URLs hiding in fields which could hold arbitrary string data (such as form fields, tab data set by extensions, etc.). If we don't care about those, the needed iteration gets quite obvious.

The question is: Should we care, considering that extensions won't be able to adjust the behavior (i.e. rather be too purge-happy or too conservative)?

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 357884 [details] [diff] [review]
browser:purge-domain-data (without RegExp)

Yeah, I think this is better. We can't keep track of everything extensions store anyways, so we'll have to rely on them making use of the observer notification.

>diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js

>+    case "browser:purge-domain-data":
>+      // remove all closed tabs containing a reference to the given domain
>+      for (let ix in this._windows) {
>+        let closedTabs = this._windows[ix]._closedTabs;
>+        for (let i = closedTabs.length - 1; i >= 0; i--) {
>+          let containsDomain = false;
>+          closedTabs[i].state.entries.forEach(function(aEntry) {
>+            if (this._getURIFromString(aEntry.url).host.hasRootDomain(aData))
>+              containsDomain = true;
>+            if (!containsDomain && aEntry.children)
>+              aEntry.children.forEach(arguments.callee, this);
>+          }, this);
>+          if (containsDomain)
>+            closedTabs.splice(i, 1);

Could simplify this using Array.some() rather than forEach().

>+// see nsPrivateBrowsingService.js
>+String.prototype.hasRootDomain = function hasRootDomain(aDomain)

I don't really like that we're spreading the pattern of doing string comparisons like this. For example, it's not clear how IDN is handled. Ideally I think we'd consolidate the "host matches, or is a sub-domain" checks in one place, and have them deal with nsIURIs rather than strings, but I guess that is best left to a followup at this point.

>diff --git a/browser/components/sessionstore/test/browser/browser_464199.js b/browser/components/sessionstore/test/browser/browser_464199.js

>+    ok(closedTabs.length == 10 &&
>+       countByTitle(closedTabs, FORGET) == 6 &&
>+       countByTitle(closedTabs, REMEMBER) == 4,
>+       "Closed tab list set up");

>+    ok(closedTabs.length == 4 &&
>+       countByTitle(closedTabs, FORGET) == 0 &&
>+       countByTitle(closedTabs, REMEMBER) == 4,
>+       "Closed tabs to be forgotten were indeed removed");

use seperate is() calls to make it clearer what failed, on the unlikely chance that something does?

r=me otherwise, but I'll wait to see the .some() version before stamping.

Comment 14

[Simon Bünzli]

Attachment: "some" better patch

(In reply to comment #13)
> Could simplify this using Array.some() rather than forEach().

Indeed more readable, thanks for the hint.

> I think we'd consolidate the "host matches, or is a sub-domain" checks in one
> place, and have them deal with nsIURIs rather than strings

Yeah, like in nsIURI2::domainIs (cf. nsIURI::schemeIs).

> but I guess that is best left to a followup at this point.

Care to file? ;-)

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 358088 [details] [diff] [review]
"some" better patch

>diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js

>+    case "browser:purge-domain-data":

>+      // remove all closed tabs containing a reference to the given domain
>+      for (let ix in this._windows) {
>+        let closedTabs = this._windows[ix]._closedTabs;
>+        for (let i = closedTabs.length - 1; i >= 0; i--) {
>+          if (closedTabs[i].state.entries.some(containsDomain, this))
>+            closedTabs.splice(i, 1);

Are there any chances this can be called when the _windows/_closedTabs objects are not initialized?

Worth adding a test for the invalid host case? about:license is a suitable test page.

Comment 16

[Simon Bünzli]

Attachment: for check-in

(In reply to comment #15)
> Are there any chances this can be called when the _windows/_closedTabs objects
> are not initialized?

No, both are initialized at the earliest possible moment (i.e. component creation and window registration).

> Worth adding a test for the invalid host case?

Done.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Blockers don't need explicit approval. Land on trunk first, then assuming the boxes cycle green, land on 1.9.1 ASAP, thanks.

Comment 18

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/203773a7fb72

Comment 19

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e39f9bc936de

Comment 20

[Marcia Knous [:marcia]]

verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090202 Shiretoko/3.1b3pre. I used the steps in Comment 0 to verify. A quick check indicates there is no Litmus test for this so nominating.

Comment 21

[Marcia Knous [:marcia]]

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090202 Minefield/3.2a1pre.

Comment 22

[Simon Bünzli]

(In reply to comment #20)
> there is no Litmus test for this so nominating.

The patch contained unit tests, so Litmus tests should not be needed.