Closed
Bug 464199
Opened 16 years ago
Closed 16 years ago
Forget About this Site doesn't forget about recently closed tabs
Categories
(Firefox :: Session Restore, defect, P1)
Firefox
Session Restore
Tracking
()
VERIFIED
FIXED
Firefox 3.1b3
People
(Reporter: beltzner, Assigned: zeniko)
References
Details
(Keywords: verified1.9.1)
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•16 years ago
|
||
(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•16 years ago
|
||
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•16 years ago
|
||
Note to self: We'll also have to make sure to clear out the respective cookies.
Reporter | ||
Comment 4•16 years ago
|
||
(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.
Reporter | ||
Updated•16 years ago
|
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Reporter | ||
Updated•16 years ago
|
Assignee | ||
Comment 5•16 years ago
|
||
Assignee | ||
Comment 6•16 years ago
|
||
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•16 years ago
|
Whiteboard: [has patch][needs review gavin]
Reporter | ||
Updated•16 years ago
|
Priority: -- → P1
Comment 7•16 years ago
|
||
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•16 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?
Comment 9•16 years ago
|
||
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 11•16 years ago
|
||
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•16 years ago
|
||
(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 13•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #354163 -
Attachment is obsolete: true
Attachment #354163 -
Flags: review?(gavin.sharp)
Updated•16 years ago
|
Whiteboard: [has patch][needs review gavin] → [needs new patch]
Assignee | ||
Comment 14•16 years ago
|
||
(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•16 years ago
|
Whiteboard: [needs new patch] → [has patch][needs review gavin]
Comment 15•16 years ago
|
||
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+
Updated•16 years ago
|
Whiteboard: [has patch][needs review gavin] → [has patch]
Reporter | ||
Updated•16 years ago
|
Whiteboard: [has patch] → [needs landing][needs to add test]
Assignee | ||
Comment 16•16 years ago
|
||
(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•16 years ago
|
Keywords: checkin-needed
Whiteboard: [needs landing][needs to add test] → [needs landing][needs baking][needs approval]
Reporter | ||
Comment 17•16 years ago
|
||
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]
![]() |
||
Comment 18•16 years ago
|
||
Pushed to mozilla-central http://hg.mozilla.org/mozilla-central/rev/203773a7fb72
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
![]() |
||
Updated•16 years ago
|
Whiteboard: [needs landing] → [needs mozilla-1.9.1 landing]
![]() |
||
Comment 19•16 years ago
|
||
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]
![]() |
||
Updated•16 years ago
|
Target Milestone: --- → Firefox 3.1b3
Comment 20•16 years ago
|
||
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
Comment 21•16 years ago
|
||
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•16 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+
Updated•16 years ago
|
Flags: in-litmus?
You need to log in
before you can comment on or make changes to this bug.
Description
•