"Show Cookies" fails, logs NSArray exception to Console

VERIFIED FIXED in Camino2.0

Status

Camino Graveyard
Preferences
--
minor
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: everylittlehonda, Assigned: Chris Lawson (gone))

Tracking

({fixed1.8.1.22})

Details

Attachments

(5 attachments, 5 obsolete attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.0.3) Gecko/2008092414 Firefox/3.0.3
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.0.3) Gecko/2008092414 Firefox/3.0.3

Press that button and nothing happens. The browser doesn't crash or stall, I can keep browsing; it's just that the cookies list doesn't pop up.

Reproducible: Always
The reporter and I walked through this on irc, and I've got the cookies.txt file in question.

This started happening with 1.6.3 and continues with 1.6.4, and we're throwing this exception:

2008-10-04 14:26:17.428 Camino[302] *** -[NSCFArray addObject:]: attempt to insert nil
(Assignee)

Comment 2

10 years ago
I don't know why this would have started happening with 1.6.3, but it's probably happening here:

http://mxr.mozilla.org/mozilla/source/camino/PreferencePanes/Privacy/PrivacyPane.mm#875

That's the only instance of addObject: in the cookies code related to an array (there's one other, but it's adding to an NSMutableSet instead).

Stuart, might it be possible for a cookies.txt file to be corrupted in some way such that the dataSource is valid, but that objectAtIndex: returns nil? I think that's probably what happened here.

Smokey said he was unable to reproduce the problem even with the cookies.txt file in question, which makes me think e-mailing it removed whatever the problem was. So as a workaround, the reporter can probably e-mail the cookies.txt file to him/herself and then use that file to replace the existing one without losing any data. (Make sure you quit Camino before replacing the file.)

Comment 3

10 years ago
(In reply to comment #2)
> That's the only instance of addObject: in the cookies code related to an array
> (there's one other, but it's adding to an NSMutableSet instead).

There's no guarantee that we are making the call directly.

> Stuart, might it be possible for a cookies.txt file to be corrupted in some way
> such that the dataSource is valid, but that objectAtIndex: returns nil? I think
> that's probably what happened here.

No. It's impossible for objectAtIndex: to return nil, because you can't store nil in an NS* collection.
I tried to reproduce this several times with the cookies.txt file that caused the problem, and I was never able to do so :(

Unless we can find some other probable cause via code inspection, we should probably close this WFM.
(Assignee)

Updated

9 years ago
Summary: cookies pane doesn't pop up when i press the "Show Cookies" button in the "Privacy" pane of Camino's Preferences window → "Show Cookies" fails, logs NSArray exception to Console
(Assignee)

Updated

9 years ago
Hardware: PowerPC → All
(Assignee)

Comment 6

9 years ago
I can't repro this any more (see the dupe), since quitting Camino (and logging out, and doing some other unrelated things) and re-launching it. Unfortunately, that probably means I can't get any useful information from a debug build either.

I agree that unless we can find a consistently reproducible scenario, we should probably just close this WORKSFORME and call it good. I'm not convinced there's anything we can do in our code to prevent it from happening based on comment 3, and I certainly have no idea how I managed to get into this situation in the first place (nor do I know why quitting Camino magically fixed it).

Comment 7

9 years ago
I just noticed I'm seeing the same issue (as the dupe): when I click on Show Cookies... in the Privacy prefpane, nothing happens except an error in Console. If I drag cookies.sqlite out of my Camino profile and relaunch, it works, ie. the dropdown sheet appears (it's blank, of course). I first noticed this using 2.0b2, got the same results with 2.0b1, and with latest nightly 2.0b3pre. 

At first Console was reporting this:

3/1/09 1:30:43 PM Camino[5497] Mozilla has caught an Obj-C exception [NSInvalidArgumentException: *** -[NSCFArray insertObject:atIndex:]: attempt to insert nil] 

Now I'm getting this:

3/1/09 2:15:10 PM Camino[5668] *** -[NSCFArray insertObject:atIndex:]: attempt to insert nil 

FWIW, my pref is set to Accept Cookies only from sites I visit and the Ask me before accepting each cookie box is not checked.
(Assignee)

Comment 8

9 years ago
Can you e-mail the problematic cookies.sqlite file to one of us (and yourself, to see if the problem magically goes away when you do)?

cl

Comment 9

9 years ago
Okay, I emailed the file to you, cl. I emailed it to myself, put the emailed version into my profile and relaunched, no magic -- I'm still not getting a Show Cookies... sheet.
(Assignee)

Comment 10

9 years ago
Good; the only time we've had this happen prior to yours, it hasn't been reproducible :(

I'll see if I can repro with your file, and if so, I can do some more debugging.
(Assignee)

Comment 11

9 years ago
What's happening here is Core is somehow allowing a cookie to be stored in the database with no name (i.e., the "name" field for the cookie is null). That explains why the problem disappeared when the original reporter e-mailed Smokey the problematic cookies.txt file, and also explains why it's not going away in the sqlite file.

Lisa, the problematic cookie came from circusponies.com. You can remove it from your file with SQLite Database Browser if you wish. However, visiting that site will cause it to come back.

I don't know how Firefox manages to avoid this problem. It appears to store the cookie with a null "name" field as well, but manages to display it without issue.

http://mxr.mozilla.org/mozilla/source/camino/src/embedding/CHCookieStorage.mm#71

may be where we're running into this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 12

9 years ago
I've filed bug 480882 and bug 480881 on Core allowing null attributes and allowing nsCookie getters to return null, respectively.

I think the short-term Camino fix for this is probably to null-check all the values we're setting in the NSHTTPCookie, but it'd be nice if we didn't have to be paranoid about what Core is giving us.

I don't know what effect storing a non-null name would have on deleting that cookie later on. (Note that |deleteCookie:| uses the cookie name in its deletion. Again, Firefox 3 somehow manages to handle this case without problems.) I plan to investigate that in a little bit.

Comment 13

9 years ago
Just verifying that after I removed the circusponies.com cookies, I now get the Show Cookies... sheet. Thanks, Chris.
Twiddling flags now that we know what's going on; it would be nice if we could have this fixed for 1.6.7 in the next few weeks.
Flags: camino2.0?
Flags: camino1.6.7?
(Assignee)

Comment 15

9 years ago
Created attachment 364861 [details]
testcase

Here's a reduced testcase. Just open this testcase, then try to open cookies.

What's happening here is an artifact of our switch to NSHTTPCookie (for display purposes only) during the de-Geckoization of our pref panes. From the NSHTTPCookie docs:

"|cookieWithProperties:| Creates and initializes an NSHTTPCookie object using the provided properties.

Return Value: The newly created cookie object. Returns nil if the provided properties are invalid."

Required properties include NSHTTPCookieName (the lack of which was behind Lisa's ability to reproduce the bug), NSHTTPCookieValue, and either NSHTTPCookieDomain *or* NSHTTPCookieOriginURL.

If any of these properties aren't provided, we fail to create the cookie, and the exception arises in CHCookieStorage.mm's |cookies| method, where we attempt to insert the created cookie into the cookie array. This fails, because |cookieWithProperties:| returned nil, and we can't add nil to an array.

Since this is just for display purposes, I think the best solution is probably to use a placeholder string for null data (heck, we could even fake it with a string consisting of whitespace) so that we get back to the behaviour we had prior to Camino 1.6.

(Personally, I still think it's silly that Core even allows the creation of cookies with null names, hosts, or values. Safari uses NSHTTPCookie and simply refuses to create such cookies.)
(Assignee)

Comment 16

9 years ago
Created attachment 364870 [details]
testcase with null domain, too

Even more fun.
Attachment #364861 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #364861 - Attachment is obsolete: false
(Assignee)

Comment 17

9 years ago
Created attachment 364922 [details]
named cookie with null domain

Just for completeness...
(Assignee)

Comment 18

9 years ago
Created attachment 364933 [details] [diff] [review]
fix v1.0

Here's a fix. Pink gave IRC approval for the substitution approach.

I left the logging code in there because this sort of thing is something we should be discouraging webmasters from doing, and it'd be useful for people to be able to say, "Hey, your site is setting broken cookies". If we'd rather not have that, though, I won't object.
Assignee: nobody → cl-bugs-new
Status: NEW → ASSIGNED
Attachment #364933 - Flags: review?(stuart.morgan+bugzilla)

Comment 19

9 years ago
Comment on attachment 364933 [details] [diff] [review]
fix v1.0

>-  [properties setObject:[NSString stringWithCString:val.get()] forKey:NSHTTPCookieDomain];
>+  NSString* cookieDomain = [NSString stringWithUTF8String:val.get()];
>+  [properties setObject:cookieDomain forKey:NSHTTPCookieDomain];

Why make this two lines?

>+    NSLog(@"Found cookie for %@ domain with null 'name' property, inserting placeholder in dictionary!", cookieDomain);

We don't want the logging; users can't fix this problem, so yelling at them about it every time they open their cookies list is just annoying. The two people in the universe who want to go after webmaster can filter their cookie listing for "<null>".

>+  if ([cookie name] == @"<null>")

If you are using this twice, it should be a static constant in this file.

>+    cookieName.Assign([[NSString string] UTF8String]);

This seems like a long way to write ""...

>+  // Danger, Will Robinson! If a host had set both a nameless cookie and
>+  // a cookie named "<null>" that share a domain and path, we will delete BOTH
>+  // cookies here. Sites being that stupid probably deserve what they get.

Do we believe this will actually ever happen? I'm not sure it warrants a comment.
Attachment #364933 - Flags: review?(stuart.morgan+bugzilla) → review-
(Assignee)

Comment 20

9 years ago
Created attachment 364964 [details] [diff] [review]
fix v1.1

Removes the logging, the Danger! comment, and makes the placeholder a static const char* const.
Attachment #364933 - Attachment is obsolete: true
Attachment #364964 - Flags: review?(stuart.morgan+bugzilla)
(Assignee)

Comment 21

9 years ago
Created attachment 365007 [details] [diff] [review]
fix v1.11

Now with more Cocoa.
Attachment #364964 - Attachment is obsolete: true
Attachment #365007 - Flags: review?(stuart.morgan+bugzilla)
Attachment #364964 - Flags: review?(stuart.morgan+bugzilla)

Comment 22

9 years ago
Comment on attachment 365007 [details] [diff] [review]
fix v1.11

>+  if (cookieName == kCookieNullPlaceholderString)

This is incorrect (although it will likely work out for internal NSString optimization reasons). The test should be [cookieName isEqualToString:kCookieNullPlaceholderString]

r=smorgan with that change.
Attachment #365007 - Flags: review?(stuart.morgan+bugzilla) → review+
(Assignee)

Comment 23

9 years ago
Created attachment 365128 [details] [diff] [review]
fix v1.12

(In reply to comment #22)
> >+  if (cookieName == kCookieNullPlaceholderString)
> 
> This is incorrect (although it will likely work out for internal NSString
> optimization reasons). The test should be [cookieName
> isEqualToString:kCookieNullPlaceholderString]

Oh, yeah, duh. That's what happens when I take a month off from thinking about Cocoa.
Attachment #365007 - Attachment is obsolete: true
Attachment #365128 - Flags: superreview?(mikepinkerton)
+  // Core doesn't and will happily set nameless cookies. Substitute a placenolder

placeholder

+  if (cookieName == kCookieNullPlaceholderString)
+    cookieName = @"";

Did you actually fix this? I don't see it addressed in the updated patch (1.12).

sr=pink with those addressed.
(Assignee)

Comment 25

9 years ago
Created attachment 365454 [details] [diff] [review]
fix v1.12 for real

Yeah, apparently I fixed it and then uploaded the same broken patch again :-p This one actually has the fix in it; I checked. :)
Attachment #365128 - Attachment is obsolete: true
Attachment #365454 - Flags: superreview+
Attachment #365454 - Flags: review+
Attachment #365128 - Flags: superreview?(mikepinkerton)
Landed on cvs trunk and MOZILLA_1_8_BRANCH for Camino 1.6.7.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: camino2.0?
Flags: camino2.0+
Flags: camino1.6.7?
Flags: camino1.6.7+
Keywords: fixed1.8.1.21
Resolution: --- → FIXED
Target Milestone: --- → Camino2.0
(Assignee)

Updated

9 years ago
Duplicate of this bug: 482202
(Assignee)

Comment 28

9 years ago
Verified on trunk per comment 2 of the dupe, and verified on branch with Version 1.6.7pre (1.8.1.22pre 2009030901).
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.21 → fixed1.8.1.22
(Assignee)

Comment 29

9 years ago
Created attachment 373440 [details] [diff] [review]
secondary fix v1.0

We had two reports of this bug coming back on the forum. I made a logging build for one of the reporters, which logged this Console snippet just before it failed:

2009-04-14 11:28:40.954 Camino[11068] Trying to make an nsICookie2 with value of [`] into an NSHTTPCookie...

2009-04-14 11:28:40.954 Camino[11068] Trying to set REQUIRED NSHTTPCookieDomain property to [¸]

2009-04-14 11:28:40.954 Camino[11068] Trying to set optional NSHTTPCookiePath property to [/]

2009-04-14 11:28:40.954 Camino[11068] Trying to set REQUIRED NSHTTPCookieValue property to [86448027.1236471278.16.6.utmcsr=104195285810180.o.hi5modules.com|utmccn=(referral)|utmcmd=referral|utmcct=/gadgets/ifr]

2009-04-14 11:28:40.954 Camino[11068] Trying to set optional NSHTTPCookieSecure property to [FALSE]

2009-04-14 11:28:40.954 Camino[11068] Trying to set optional NSHTTPCookieExpires (expiration date) property to [2009-09-06 13:14:37 +0100]

2009-04-14 11:28:40.954 Camino[11068] About to return new cookie: [(null)]

2009-04-14 11:28:40.955 Camino[11068] Trying to add [(null)] to cookies array...

2009-04-14 11:28:40.963 Camino[11068] *** -[NSCFArray addObject:]: attempt to insert nil 

I don't know why that particular cookie failed, and once it was deleted, the reporter couldn't replicate the behaviour by re-visiting the site in question.

I think we should keep the fix we already took, since that allows "invalid" cookies to work properly. I think we should take another fix, however: one that does not assume cookieFromGeckoCookie: will return a valid cookie. This will result in some very limited subset of cookies failing to display (and therefore being individually impossible to delete without editing the sqlite file manually), but I think it's an overall win for users because they won't ever get this sort of behaviour again. I think it's more important that the "Show Cookies" button work than it is for every single cookie to display in the table.

If we can ever figure out exactly what about that cookie was causing |cookieFromGeckoCookie:| to return nil, we should take a fix specific for that problem if at all possible. I was never able to repro the issue with either the cookies.txt or .sqlite file in question.
Attachment #373440 - Flags: review?(stuart.morgan+bugzilla)
(Assignee)

Comment 30

9 years ago
Reopening per comment 29.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Comment 31

9 years ago
(In reply to comment #29)
> I think we should take another fix, however: one that does not assume
> cookieFromGeckoCookie: will return a valid cookie.

Yep, absolutely. Thanks for tracking that down!

Comment 32

9 years ago
Comment on attachment 373440 [details] [diff] [review]
secondary fix v1.0

>+      // Instead of refusing to display cookies at all and logging an NSArray exception,
>+      // let's just not display the single problem cookie.

Remove these two lines, as it's self-explanatory that adding nil to an array is undesirable.

r/sr=smorgan with that change.
Attachment #373440 - Flags: review?(stuart.morgan+bugzilla) → superreview+
(Assignee)

Comment 33

9 years ago
Created attachment 373444 [details] [diff] [review]
secondary fix v1.01

For checkin.
Attachment #373440 - Attachment is obsolete: true
Attachment #373444 - Flags: superreview+
Attachment #373444 - Flags: review+
(In reply to comment #33)
> Created an attachment (id=373444) [details]
> secondary fix v1.01
> 
> For checkin.

Checked in on cvs trunk.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Flags: camino1.6.8?
Resolution: --- → FIXED

Comment 35

9 years ago
(In reply to comment #29)
> 2009-04-14 11:28:40.954 Camino[11068] Trying to set REQUIRED NSHTTPCookieDomain
> property to [¸]


For the record: I would guess this would be the culprit, but even if we demonstrate that it is I don't see that there's much we could do beyond this patch.
(Assignee)

Comment 36

9 years ago
(In reply to comment #35)
> (In reply to comment #29)
> > 2009-04-14 11:28:40.954 Camino[11068] Trying to set REQUIRED NSHTTPCookieDomain
> > property to [¸]

> 
> For the record: I would guess this would be the culprit, but even if we
> demonstrate that it is I don't see that there's much we could do beyond this
> patch.

I would have thought so, too, but the previous cookie in his list, which succeeded, had this:

2009-04-14 11:28:40.953 Camino[11068] Trying to make an nsICookie2 with value of [`] into an NSHTTPCookie...
2009-04-14 11:28:40.953 Camino[11068] Trying to set REQUIRED NSHTTPCookieDomain property to [¸]
2009-04-14 11:28:40.953 Camino[11068] Trying to set optional NSHTTPCookiePath property to [/]
2009-04-14 11:28:40.953 Camino[11068] Trying to set REQUIRED NSHTTPCookieValue property to [10902101755271274283]
2009-04-14 11:28:40.953 Camino[11068] Trying to set optional NSHTTPCookieSecure property to [FALSE]
2009-04-14 11:28:40.953 Camino[11068] Trying to set optional NSHTTPCookieExpires (expiration date) property to [2019-02-10 22:55:27 +0000]
2009-04-14 11:28:40.954 Camino[11068] About to return new cookie: [<NSHTTPCookie version:0 name:@"KMVisitor" value:@"10902101755271274283" expiresDate:@"2019-02-10 22:55:27 +0000" created:@"261397720.952914" sessionOnly:FALSE domain:@".tracking.summitmedia.co.uk" path:@"/" secure:FALSE comment:@"(null)" commentURL:@"(null)" portList:(null)>]
2009-04-14 11:28:40.954 Camino[11068] Trying to add [<NSHTTPCookie version:0 name:@"KMVisitor" value:@"10902101755271274283" expiresDate:@"2019-02-10 22:55:27 +0000" created:@"261397720.952914" sessionOnly:FALSE domain:@".tracking.summitmedia.co.uk" path:@"/" secure:FALSE comment:@"(null)" commentURL:@"(null)" portList:(null)>] to cookies array...

I think my logging code was wrong :\

cl
(In reply to comment #33)
> Created an attachment (id=373444) [details]
> secondary fix v1.01

Checked this in on the MOZILLA_1_8_BRANCH in advance of 1.6.8 so that we can point anyone who needs it to branch nightlies.
Flags: camino1.6.8? → camino1.6.8+

Comment 39

9 years ago
Recent nightly does show cookie sheet when using my bad cookies, obviously they don't show up in the list.
From what I can tell, the only possible difference with the bad cookies is that in the host address a "_" (underscore) is used. Could that be the error?
(Assignee)

Comment 40

9 years ago
(In reply to comment #39)
> Recent nightly does show cookie sheet when using my bad cookies, obviously they
> don't show up in the list.
> From what I can tell, the only possible difference with the bad cookies is that
> in the host address a "_" (underscore) is used. Could that be the error?

It's possible, since the underscore is not a valid character in domains. I'll look into this more tomorrow; if that's it, I can work up a workaround patch that'll allow these cookies to be displayed.
(Assignee)

Comment 41

9 years ago
I did some testing tonight and wrote up a new logging patch. Hopefully Richard will have some results for us in a day or so with that, but in the meantime...

It looks like a null path could also cause this. The NSHTTPCookie documentation never says anything about it, but if NSHTTPCookieDomain is specified (rather than NSHTTPCookieOriginURL), NSHTTPCookiePath is *also* a required property. See here for more:

http://lists.apple.com/archives/Webkitsdk-dev//2004/Apr/msg00014.html

Once we hear back from Richard, I should be able to make a determination of what's going on here and make Yet Another Patch.
(Assignee)

Comment 42

9 years ago
Richard got back to me. It's not a lack of a path, and it's not any problem with the date, which pretty much exhausts our list of possibilities. I also can't replicate the failure when manually setting a cookie using the logged values.

I'd say it might be a problem with the OS itself, since I was never able to repro the problem even with the "broken" sqlite file. There have been three independent reports of this so far, but it's possible all three of them are hitting the same bug/brokenness.

The only other thing I can think of is that somehow, a null character might have ended up in the file somewhere and that's breaking things. I'm not sure how we might detect or prevent that, or if it's even possible given all the translation between Gecko APIs and Cocoa APIs that we do in there.
(Assignee)

Comment 43

9 years ago
OK, I just did some more testing, and this is either a 10.4 thing or a PPC thing. I need to install 10.5 on my PBG4 and I'll be able to narrow that down.
(Assignee)

Comment 44

9 years ago
Yep, this is 10.4-only. For whatever reason, the NSHTTPCookie API fails to create certain otherwise-valid cookies on 10.4.

Unless there's a compelling reason to do otherwise, I think we're OK with the current situation (simply not displaying the problem cookies on 10.4 machines), since we don't know what exactly about them is causing NSHTTPCookie's initializer to fail. If we knew what triggered the bug, it might be worth working around it, but I doubt there's anything we can do without knowing how to trigger it.

VERIFIED FIXED in the 25 Apr 09 trunk nightly on PPC, as far as the failure to display a cookie sheet is concerned. Richard, thanks for all your help.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.