Forget button can fail to clear cookies by running sanitizer too early

VERIFIED FIXED in Firefox 33

Status

()

Firefox
Toolbars and Customization
--
major
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: tracy, Assigned: Dolske)

Tracking

33 Branch
Firefox 36
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox33 verified, firefox34 verified, firefox35 verified, firefox36 verified)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
From flod's test challenge report

1) Cleared all history, verified that no cookies were stored, closed the Info dialog.
2) Opened www.corriere.it, load a ton of ads and cookies.
3) Forget last 5 minutes. Result: history is gone, 4 cookies remain.

Explaining the procedure since I've seen suggestions that cookies could exists prior to the 5 minutes timeframe, or people were keeping the Info dialog open.
(Reporter)

Comment 1

3 years ago
I can reproduce with www.corriere.it

4 cookies remain under corrieri.it in the cookies manager.
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

3 years ago
Blocks: 1069300
(Assignee)

Comment 5

3 years ago
Hmm. If I enable cookie NSPR logging (plus a few dump()s in sanitizer.js), I see a number of cookies being set *after* the sanitizer has closed windows and cleared cookies. Adding in some Necko logging, it looks like we're making network connections when that's happening. (It's a little hard to tell exactly what's happening because I'm not familiar with the logging output.)

I'd guess that tracking scripts on the page are running when it's closed (XHRs?), and those parts finish async after we think we've closed the window. :(

We should verify that, but I'm not sure how to address it if that's what is happening. Either we need to somehow wait for content to finish before clearing, or we need to poison lingering context so it can't persist data. Both seem difficult.
(Assignee)

Comment 6

3 years ago
Created attachment 8510581 [details]
Stacktrace

The browser console does show 2 network requests, although oddly neither is to the site (www.corriere.it) for the remaining cookie... After triggering Forget, I see a "GET http://metrics.rcsmetrics.it/b/ss/rcsglobal/1/H.25.2/s49713314309640" and "GET https://bam.nr-data.net/jserrors/1/3b7afaa056"

Some poking with lldb shows that the actual cookie setting seems to be done by a |pagehide| listener, setting document.cookie. (Full stack attached.)
(Reporter)

Comment 7

3 years ago
I've seen that if you run Forget (5 minutes) right away in the new window, things get cleaned up.  Would running the cookie cleanup portion of Forget after the original window has been closed do the trick, if possible?
(Assignee)

Comment 8

3 years ago
Ok, I think the change from bug 874502 is biting us here. (I previously dealt with some of that fallout in bug 917797, which is why some of this seemed familiar.) Windows are closed async from chrome, so we need to explicitly wait for that to be complete.

I think that means https://developer.mozilla.org/en-US/docs/Observer_Notifications#Windows

Hopefully we can just wait for the expected number of xul-window-destroyed notifications, and not need to track the individual tabs being closed.
(Assignee)

Comment 9

3 years ago
...Based on some quick logging, I do see xul-window-destroyed being fired after the cookie setting happens. So using that should hopefully work. Although it's a little surprising that all the inner-window-destroyed notifications happen after xul-window-destroyed, we should check with our document-lifetime wizards to see if there's anything that can manage to run between them.

Comment 10

3 years ago
Taking per mail with dolske.
Assignee: dolske → gijskruitbosch+bugs
Status: NEW → ASSIGNED

Updated

3 years ago
Iteration: --- → 36.1
Points: --- → 5
Flags: qe-verify+
Flags: firefox-backlog+
(Reporter)

Comment 11

3 years ago
nightly iteration backlog work?  I hope you're planning to fix on 33 soon. It is a show stopper for 33.1, which must be signed off in 7 days.
(Assignee)

Comment 12

3 years ago
Created attachment 8511320 [details] [diff] [review]
Patch v.0 WIP

Messy WIP, but fixes the bug. Loading www.corriere.it and using the Forget button will now correctly clear cookies after the async window closing is finished, and no cookies are shown in the cookie manager.

I'm a little confounded by a new failure in the canClear() code for formdata, though. |let transactionMgr = searchBar.textbox.editor.transactionManager;| is now throwing because |editor| is null. I just commented this code out for testing, but I don't understand what's happening. Need to either figure it out or bandaid with a null-check.

Needs a test too.
Attachment #8510581 - Attachment is obsolete: true
(Assignee)

Comment 13

3 years ago
(In reply to Justin Dolske [:Dolske] from comment #12)
> |let transactionMgr = searchBar.textbox.editor.transactionManager;| is now throwing because
> |editor| is null.

Ok, so this is another race. It's only working right now because it runs while the old window is still open. By waiting to sanitize until after the old window is well and truly dead, this code breaks. Could fix it to null-check, but we'd want to have a test for this condition (no open window) which might be tricky. Probably easier to explicitly wait for the new window to exist before sanitizing.
(Assignee)

Comment 14

3 years ago
Created attachment 8511407 [details] [diff] [review]
Patch v.1
Attachment #8511320 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Summary: some cookies aren't cleared from Forget button last 5 minutes → Forget button can fail to clear cookies by running sanitizer too early
Assignee: gijskruitbosch+bugs → dolske
(Assignee)

Comment 15

3 years ago
Created attachment 8511733 [details] [diff] [review]
Patch v.2

Updated to use the browser-delayed-startup-finished notification instead of a load event. Mostly just because bug 1079222 wants to use that too, but I suppose it's better to wait until then. And it's a bit cleaner to use 2 observers instead of 1 observer and 1 event listener.

Manually tested on Windows as well, as I wanted to make sure the state in comment 13 (old windows closed, new window not yet fully open) was correctly handled (i.e., closing the last window on Windows normally quits the app, so I wanted to make sure it correctly handles there being a window in the process of opening).

Also realized we can't test this bug in an automated and realistic fashion -- just like bug 1069300 itself -- because our test frameworks expect to be running from an open window. So manual testing will have to do.

Finally, I'd thought about converting onWindowsCleaned to be a Promise instead of a callback, but... Trying that is kind of making my head hurt instead of making it clearer what's going on (at least to me). Plus the current code already works, so I'm inclined to go with what I have to get it landed ASAP.
Attachment #8511407 - Attachment is obsolete: true
Attachment #8511733 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 16

3 years ago
Comment on attachment 8511733 [details] [diff] [review]
Patch v.2

Gijs/MattN/Jared -- whoever can get to this first, please steal. :)
Attachment #8511733 - Flags: review?(MattN+bmo)
(Assignee)

Updated

3 years ago
Attachment #8511733 - Flags: review?(jaws)
(Assignee)

Updated

3 years ago
Component: Preferences → Toolbars and Customization
Comment on attachment 8511733 [details] [diff] [review]
Patch v.2

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

First pass:

::: browser/base/content/sanitize.js
@@ +75,5 @@
>        let item = this.items.openWindows;
> +
> +      function onWindowsCleaned() {
> +        try {
> +          var clearedPromise = this.sanitize(itemsToClear);

var!

@@ +79,5 @@
> +          var clearedPromise = this.sanitize(itemsToClear);
> +          clearedPromise.then(deferred.resolve, deferred.reject);
> +        } catch(e) {
> +          Cu.reportError("Sanitizer threw after closing windows: " + e);
> +          deferred.reject();

Give a rejection reason to help with debugging.

@@ +83,5 @@
> +          deferred.reject();
> +        }
> +      }
> +
> +      var ok = item.asyncClear(onWindowsCleaned.bind(this))

var => let here too. Also missing a semicolon.

@@ +90,1 @@
>          deferred.reject();

Rejection reason here too

@@ +457,5 @@
>          for (let win of aWindowList) {
>            win.getInterface(Ci.nsIDocShell).contentViewer.resetCloseWindow();
>          }
>        },
> +      asyncClear: function(aCallback)

This rename seems like it could cause problems in the future as a "clear" method seems to be part of the undocumented interface for each item. Two ideas:
* add a |clear| method which reports an error when called and refers to asyncClear.
* keep this as |clear| but warning if |aCallback| is missing.

@@ +499,5 @@
>          let features = "chrome,all,dialog=no," + this.privateStateForNewWindow;
>          let newWindow = existingWindow.openDialog("chrome://browser/content/", "_blank",
>                                                    features, defaultArgs);
>  
> +        // Window creation and destruction is asyncronous. We need to wait

Typo: asynchronous
Comment on attachment 8511733 [details] [diff] [review]
Patch v.2

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

I'm not that familiar with xul-window-destroyed but I trust that you're testing makes sense.

::: browser/base/content/sanitize.js
@@ +504,5 @@
> +        // until all existing windows are fully closed, and the new window is
> +        // fully open, before continuing. Otherwise the rest of the sanitizer
> +        // could run too early (and miss new cookies being set when a page
> +        // closes) and/or run too late (and not have a fully-formed window yet
> +        // in existance). See bug 1088137.

typo: existence

@@ +517,5 @@
> +          if (numWindowsClosing == 0)
> +            aCallback();
> +        }
> +
> +        var numWindowsClosing = windowList.length;

s/var/let/
Attachment #8511733 - Flags: review?(jaws)
Attachment #8511733 - Flags: review?(gijskruitbosch+bugs)
Attachment #8511733 - Flags: review?(MattN+bmo)
Attachment #8511733 - Flags: review+
(Assignee)

Comment 19

3 years ago
Created attachment 8511782 [details] [diff] [review]
Patch v.3

Nits fixed.

I had used asychClose() instead of close() just to make sure it didn't get caught by something that might ignorantly call it, but having it hard-fail seems fine too.
Attachment #8511733 - Attachment is obsolete: true
(Assignee)

Comment 20

3 years ago
Also I continually misspell "async". :|
(Assignee)

Comment 21

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/61cbd994f800
https://hg.mozilla.org/mozilla-central/rev/61cbd994f800
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
(Assignee)

Comment 23

3 years ago
https://hg.mozilla.org/projects/alder/rev/860d5a053b5f
(Assignee)

Updated

3 years ago
Whiteboard: [fixed-alder]
(Assignee)

Comment 24

3 years ago
Comment on attachment 8511782 [details] [diff] [review]
Patch v.3

[Triage Comment]

Needed for correct operation of privacy button launching on an accelerated schedule.
Attachment #8511782 - Flags: approval-mozilla-beta+
Attachment #8511782 - Flags: approval-mozilla-aurora+

Comment 25

3 years ago
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/3ebeb2d211e1
status-firefox33: --- → unaffected
status-firefox34: --- → affected
status-firefox35: --- → fixed
status-firefox36: --- → fixed

Comment 26

3 years ago
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/4c8d686c690b
status-firefox34: affected → fixed

Comment 27

3 years ago
(In reply to Justin Dolske [:Dolske] from comment #21)
> https://hg.mozilla.org/integration/fx-team/rev/61cbd994f800

NB: this comment: https://hg.mozilla.org/integration/fx-team/rev/61cbd994f800#l1.5
is now wrong. Can you fix it with a DONTBUILD push? :-)
status-firefox33: unaffected → ---
status-firefox34: fixed → ---
status-firefox35: fixed → ---
status-firefox36: fixed → ---
Flags: needinfo?(dolske)
Target Milestone: Firefox 36 → ---

Comment 28

3 years ago
Ugh, bugzilla.
status-firefox33: --- → unaffected
status-firefox34: --- → fixed
status-firefox35: --- → fixed
status-firefox36: --- → fixed
Target Milestone: --- → Firefox 36
(Reporter)

Comment 29

3 years ago
verified this is fixed on the alder-nighty build for 20141028 with cookies from both www.corriere.it and nytimes.com
https://hg.mozilla.org/releases/mozilla-release/rev/860d5a053b5f
Whiteboard: [fixed-alder]
status-firefox33: unaffected → fixed
Verified on Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.8.5 using Firefox 33.1, latest Aurora and latest Nightly that cookies are deleted properly except with google.com PREF cookie in some cases (which I understand it`s a safe browsing cookie, see bug 1008706). Based on my testing and testing done by Tracy in comment 29, marking flags accordingly.
Status: RESOLVED → VERIFIED
status-firefox33: fixed → verified
status-firefox35: fixed → verified
status-firefox36: fixed → verified
Depends on: 1094429
I was able to reproduce this bug on Nightly 36.0a1 (2014-10-23) using Windows 7 x64.

Verified fixed on Windows 7 x64, Mac OSX 10.9.5 and Ubuntu 14.04 x32 using Beta 34.0b7 (20141106201515). Only google.com PREF cookie is displayed in Cookies Window.
status-firefox34: fixed → verified
(Assignee)

Comment 33

3 years ago
(In reply to :Gijs Kruitbosch from comment #27)

> NB: this comment:
> https://hg.mozilla.org/integration/fx-team/rev/61cbd994f800#l1.5
> is now wrong. Can you fix it with a DONTBUILD push? :-)

It seems accurate to me.
Flags: needinfo?(dolske)
You need to log in before you can comment on or make changes to this bug.