Last Comment Bug 358723 - Crashes [@ nsHTMLDocument::GetCookie]
: Crashes [@ nsHTMLDocument::GetCookie]
Status: RESOLVED FIXED
[need testcase]
: fixed1.8.1.1
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: 1.8 Branch
: x86 All
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-10-30 03:52 PST by Olli Pettay [:smaug]
Modified: 2008-07-31 02:45 PDT (History)
6 users (show)
dveditz: blocking1.8.1.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
use GetPrincipal (2.44 KB, patch)
2006-10-30 08:10 PST, Olli Pettay [:smaug]
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.1.1+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2006-10-30 03:52:14 PST
According to TB there are some FF2 crashes @ nsHTMLDocument::GetCookie.
nsHTMLDocument::GetCookie  [mozilla/content/html/document/src/nsHTMLDocument.cpp, line 1829]
XPTC_InvokeByIndex  [mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102]
XPCWrappedNative::CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2169]
XPC_WN_GetterSetter  [mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1487]
js_Invoke  [mozilla/js/src/jsinterp.c, line 1377]

I wonder in which case mPrincipal is null?
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLDocument.cpp&mark=1829&rev=MOZILLA_1_8_BRANCH#1829
Comment 1 Olli Pettay [:smaug] 2006-10-30 03:55:33 PST
Btw, most of the crash reports mention http://earth.google.com/earth.html or other
google earth pages.
Comment 2 Boris Zbarsky [:bz] 2006-10-30 07:33:55 PST
> I wonder in which case mPrincipal is null?

When it feels like -- mPrincipal is lazily allocated on the 1.8 branch.  All consumers should be calling GetPrincipal(), then null-checking the result and throwing OOM if it's null.
Comment 3 Olli Pettay [:smaug] 2006-10-30 07:52:04 PST
(In reply to comment #2)
> When it feels like -- mPrincipal is lazily allocated on the 1.8 branch.  All
> consumers should be calling GetPrincipal(), then null-checking the result and
> throwing OOM if it's null.
> 
Ah, ok. I can then take this and fix also other cases when principal
isn't used properly.
Comment 4 Olli Pettay [:smaug] 2006-10-30 08:00:26 PST
(In reply to comment #2)then null-checking the result and
> throwing OOM if it's null.
> 

Not necessarily OOM, as far as I see. It may fail also during shutdown.
Comment 5 Olli Pettay [:smaug] 2006-10-30 08:10:30 PST
Created attachment 244094 [details] [diff] [review]
use GetPrincipal

Like this?
Comment 6 Boris Zbarsky [:bz] 2006-10-30 09:44:12 PST
Comment on attachment 244094 [details] [diff] [review]
use GetPrincipal

Looks great.  r+sr=bzbarsky
Comment 7 Olli Pettay [:smaug] 2006-11-05 13:17:11 PST
Note, this is not needed for trunk, because NodePrincipal() is used there.
Comment 8 Daniel Veditz [:dveditz] 2006-11-20 12:30:01 PST
Comment on attachment 244094 [details] [diff] [review]
use GetPrincipal

approved for 1.8 branch, a=dveditz for drivers
Comment 9 Steve England [:stevee] 2006-11-27 15:41:02 PST
--> RESOLVED FIXED?
Comment 10 Olli Pettay [:smaug] 2006-12-04 05:41:54 PST
Yes, this should be fixed.
Comment 11 Jay Patel [:jay] 2006-12-08 15:22:33 PST
Does anyone have a simplified testcase or steps to reproduce this crash?  Would be nice to verify the fix...
Comment 12 Boris Zbarsky [:bz] 2006-12-08 20:10:01 PST
All we have is talkback data...

Note You need to log in before you can comment on or make changes to this bug.