Closed Bug 358723 Opened 13 years ago Closed 13 years ago

Crashes [@ nsHTMLDocument::GetCookie]

Categories

(Core :: DOM: Core & HTML, defect, critical)

1.8 Branch
x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

Details

(Keywords: fixed1.8.1.1, Whiteboard: [need testcase])

Attachments

(1 file)

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
Btw, most of the crash reports mention http://earth.google.com/earth.html or other
google earth pages.
> 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.
Flags: blocking1.8.1.1?
(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.
Assignee: general → Olli.Pettay
(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.
Attached patch use GetPrincipalSplinter Review
Like this?
Attachment #244094 - Flags: superreview?(bzbarsky)
Attachment #244094 - Flags: review?(bzbarsky)
Comment on attachment 244094 [details] [diff] [review]
use GetPrincipal

Looks great.  r+sr=bzbarsky
Attachment #244094 - Flags: superreview?(bzbarsky)
Attachment #244094 - Flags: superreview+
Attachment #244094 - Flags: review?(bzbarsky)
Attachment #244094 - Flags: review+
Attachment #244094 - Flags: approval1.8.1.1?
Note, this is not needed for trunk, because NodePrincipal() is used there.
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Comment on attachment 244094 [details] [diff] [review]
use GetPrincipal

approved for 1.8 branch, a=dveditz for drivers
Attachment #244094 - Flags: approval1.8.1.1? → approval1.8.1.1+
Keywords: fixed1.8.1.1
--> RESOLVED FIXED?
Yes, this should be fixed.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Does anyone have a simplified testcase or steps to reproduce this crash?  Would be nice to verify the fix...
Whiteboard: [need testcase]
All we have is talkback data...
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.