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

VERIFIED FIXED in Firefox 3.1b3

Status

()

Firefox
Session Restore
P1
normal
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: beltzner, Assigned: Simon Bünzli)

Tracking

({verified1.9.1})

Trunk
Firefox 3.1b3
verified1.9.1
Points:
---
Bug Flags:
blocking-firefox3.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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
Flags: blocking-firefox3.1?
(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)
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.
(Assignee)

Comment 3

9 years ago
Note to self: We'll also have to make sure to clear out the respective cookies.
Component: General → Session Restore
Depends on: 461634
QA Contact: general → session.restore
(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.
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Depends on: 460086
No longer depends on: 461634
(Assignee)

Comment 5

9 years ago
Created attachment 353834 [details] [diff] [review]
WIP
(Assignee)

Comment 6

9 years ago
Created attachment 354163 [details] [diff] [review]
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.).
Assignee: nobody → zeniko
Attachment #353834 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #354163 - Flags: review?(gavin.sharp)
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review gavin]
Priority: -- → P1
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
(Assignee)

Comment 8

9 years ago
(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?
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.
Gavin: can you expedite review here or reassign?
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 :)
(Assignee)

Comment 12

9 years ago
Created attachment 357884 [details] [diff] [review]
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)?
Attachment #357884 - Flags: review?(gavin.sharp)
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.
Attachment #357884 - Flags: review?(gavin.sharp)
Attachment #354163 - Attachment is obsolete: true
Attachment #354163 - Flags: review?(gavin.sharp)
Whiteboard: [has patch][needs review gavin] → [needs new patch]
(Assignee)

Comment 14

9 years ago
Created attachment 358088 [details] [diff] [review]
"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? ;-)
Attachment #357884 - Attachment is obsolete: true
Attachment #358088 - Flags: review?(gavin.sharp)
(Assignee)

Updated

9 years ago
Whiteboard: [needs new patch] → [has patch][needs review gavin]
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.
Attachment #358088 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][needs review gavin] → [has patch]
Whiteboard: [has patch] → [needs landing][needs to add test]
(Assignee)

Comment 16

9 years ago
Created attachment 358210 [details] [diff] [review]
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.
Attachment #358088 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: [needs landing][needs to add test] → [needs landing][needs baking][needs approval]
Blockers don't need explicit approval. Land on trunk first, then assuming the boxes cycle green, land on 1.9.1 ASAP, thanks.
Whiteboard: [needs landing][needs baking][needs approval] → [needs landing]
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/203773a7fb72
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs mozilla-1.9.1 landing]
Pushed to mozilla-1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e39f9bc936de
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [needs mozilla-1.9.1 landing]
Target Milestone: --- → Firefox 3.1b3
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.
Flags: in-litmus?
Keywords: fixed1.9.1 → verified1.9.1
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.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 22

9 years ago
(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.
Flags: in-testsuite+
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.