If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

failure to load cookie service causes crash in nsHTMLDocument::GetCookie()

VERIFIED FIXED in mozilla0.9.2

Status

()

Core
Networking: Cookies
P1
critical
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: Ken, Assigned: Stephen P. Morse)

Tracking

({crash})

Trunk
mozilla0.9.2
x86
Windows 2000
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: critical for 0.9.2 embedding; r=morse, sr=scc, a=chofmann, URL)

Attachments

(2 attachments)

(Reporter)

Description

17 years ago
From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)
BuildID:    

I am embedding gecko under Windows. In nsHTMLDocument.cpp, 
nsHTMLDocument::GetCookie() makes the assumption that the cookie service works. 
Unfortunately, the code added for revision 3.322 by morse@netscape.com will 
cause a crash if it doesn't.

Basically, a char* was not initialized to nsnull, but my proposed patch cleans 
things up a little more.


Reproducible: Always
Steps to Reproduce:
1. If you have the cookie service, remove it from the components
2. Load any website that uses cookies
3. Crash will occur on nsCRT::free(cookie)

Expected Results:  Do not assume cookie service can load. First truncate the 
referenced aCookie, then attempt to retrieve the new one and assign/free only 
if successful.
(Reporter)

Comment 1

17 years ago
Created attachment 39334 [details] [diff] [review]
patch to revision 3.348

Updated

17 years ago
Keywords: crash

Comment 2

17 years ago
steve, can you have a look?  we need this for embedding.  thanks
Whiteboard: critical for embedding 0.9.2
Target Milestone: --- → mozilla0.9.2

Comment 3

17 years ago
These changes look fine to me (they are safer than the previous code, but 
appear to be equivalent in all other respects).

Looking at the code, I suspect that we've turned over a rock where a bunch of 
I18n issues are crawling. Can cookies contain non-ASCII characters?

Also, it sort of sucks that we have to copy the string twice to return it. scc, 
any suggestions for the string-fu?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 4

17 years ago
No, cookies cannot contain non-ascii characters.  There is no i18n problem here.
(Assignee)

Comment 5

17 years ago
r=morse on the patch
(Assignee)

Comment 6

17 years ago
Waterson, since you already looked this over, would you like to super-review it?
Status: NEW → ASSIGNED

Comment 7

17 years ago
I'll let scc do the honors, as he may have some ideas for improving the string 
usage.
+      str.AssignWithConversion(cookie);
+      nsCRT::free(cookie);
+      aCookie.Assign(str);

Should be:

CopyASCIItoUCS2(nsDependentCString(cookie), aCookie);
nsMemory::free(cookie);

to eliminate one string copy.

Comment 9

17 years ago
> No, cookies cannot contain non-ascii characters.  There is no i18n
> problem here.

Well, with the APIs that we have, people can certainly _set_ cookies with 
non-ASCII characters:

 nsHTMLDocument::SetCookie(const nsAReadableString& aCookie)

So, is the right behavior here to just completely corrupt the data (at line 
nsHTMLDocument.cpp:1992)? (Oy, is the right behavior to allocate a new 
PRUnichar buffer with the nsString() ctor, copy the data into the buffer, and 
then corrupt it?)

Gratuitous copies aside, I'd have thought UTF-8 encoding it might be more 
polite. Anyway, if any of the intl guys want to take issue with that, let's file 
another bug.
How about changing:

    char *cookie = nsString(aCookie).ToNewCString();

to

    char *cookie = ToNewCString(aCookie);

in ::SetCookie() to eliminate one extra malloc and string copy while we're at
it? Or we could just as easily make that ToNewUTF8String(aCookie) and make
::GetCookie() code use NS_ConvertUTF8toUCS2() (which does one extra unavoidable
string copy) to fix the unicode dataloss problems?

Comment 11

17 years ago
> Gratuitous copies aside, I'd have thought UTF-8 
> encoding it might be more polite. Anyway, if any of 
> the intl guys want to take issue with that, let's file 
> another bug.

I think the relevant RFC is the one above. The 'value'
according to this RFC could be anything the server wants
to set. But if we do any conversion on our side, how do we
make sure that the stored value matches that expected
by the server at the next visit? (Let's move this to 
another bug...)

Comment 12

17 years ago
nav triage team:

Marking nsbeta1+ and p1.
Keywords: nsbeta1+
Priority: -- → P1

Updated

17 years ago
Keywords: nsBranch
This ia 0.9.2 bug.  What's the deal here?
(Assignee)

Comment 14

17 years ago
Awaiting for super-review.
Whiteboard: critical for embedding 0.9.2 → critical for embedding 0.9.2; r=morse, sr=?, a=?

Comment 15

17 years ago
Created attachment 39959 [details] [diff] [review]
similar patch, fewer copies, still no UTF-8 support
(Assignee)

Comment 16

17 years ago
r=morse for waterson's patch
still awaiting sr from scc.

Comment 17

17 years ago
I like the patch, the only thing I might change is that I would try to manage
the life of the |char*| at the end automatically, e.g.,

  nsXPIDLCString cookie;
  cookie.Adopt(ToNewCString(aCookie));
  if ( cookie.get() )
    ...

I don't feel strongly enough about this to deny you an sr, but if this feels
right to you, go ahead and add it.  We're just punting on UTF8?

sr=scc
(Assignee)

Updated

17 years ago
Whiteboard: critical for embedding 0.9.2; r=morse, sr=?, a=? → critical for embedding 0.9.2; r=morse, sr=scc, a=?

Comment 18

17 years ago
a=chofmann branch and trunk
(Assignee)

Comment 19

17 years ago
Fix checked in to both trunck and branch
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: critical for embedding 0.9.2; r=morse, sr=scc, a=? → critical for embedding 0.9.2; r=morse, sr=scc, a=chofmann
Whiteboard: critical for embedding 0.9.2; r=morse, sr=scc, a=chofmann → critical for 0.9.2 embedding; r=morse, sr=scc, a=chofmann

Comment 20

16 years ago
verified: win2k 2001082303
Status: RESOLVED → VERIFIED
Keywords: nsBranch → nsbranch
You need to log in before you can comment on or make changes to this bug.