605835 - Stack exhaustion in cookie observers on startup with 10-20 nightly

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Stack

I can't launch my nightly today on mac. See attached stack, we're doing something recursive in cookie land now. I even renamed my old cookies.sqlite file to see if that helped but I still crash in the same way.

Comment 1

[[e-mail]]

DumpJSStack()?

Comment 2

[[e-mail]]

So, today's nightly is also very crashy for me, but it's a crash in JS (bug 605452) which was fixed on trunk an hour ago. (Note that I can't reproduce on my own debug build with the rev of last night's nightly, but I can with opt.)

Can you roll an opt build of current tip, or download an hourly, and retest?

Meantime, I'll look through the observers in our tree and see if anything leaps out at me.

Comment 3

[[e-mail]]

Note that this could also be an addon -- what do you have installed?

Comment 4

[[e-mail]]

And can you repro on a debug build?

Comment 5

[[e-mail]]

Attachment: patch

Upon code inspection, I did find a bug in how we handle stale cookies. It may well be the cause of this, since it affects how notifications are dispatched.

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 484745 [details] [diff] [review]
patch

>+++ b/netwerk/cookie/nsCookieService.cpp
>+    bool isStale = oldCookie->Expiry() <= currentTime;
move this below the comment, right above the if.

>+    // Check if the old cookie is stale (i.e. has already expired). If so, we
>+    // need to be careful about the semantics of removing it and adding the new
>+    // cookie.
Why?  Please explain.

>+    if (isStale) {
>+      if (aCookie->Expiry() <= currentTime) {
>+        // The new cookie has expired and the old one is stale. Nothing to do.
>+        COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader,
>+          "cookie has already expired");
nit: line up with (

>+      // If the old cookie is httponly, make sure we're not coming from script.
>+      if (!aFromHttp && oldCookie->IsHttpOnly()) {
>+        COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader,
>+          "previously stored cookie is httponly; coming from script");
ditto

>+      COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader,
>+        "previously stored cookie was deleted");
and here

>+      // If the new cookie has expired -- i.e. the intent was simply to delete
>+      // the old cookie -- then we're done.
>+      if (aCookie->Expiry() <= currentTime)
>+        return;
nit: brace this if please

>     if (aCookie->Expiry() <= currentTime) {
>-      COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader, "cookie has already expired");
>+      COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader,
>+        "cookie has already expired");
nit: line up with (

r=sdwilsh

Comment 7

[[e-mail]]

Done.

http://hg.mozilla.org/mozilla-central/rev/9bd3db0de90e