358723 - Crashes [@ nsHTMLDocument::GetCookie]

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

Btw, most of the crash reports mention http://earth.google.com/earth.html or other
google earth pages.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> 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][[e-mail]]]

(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][[e-mail]]]

(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][[e-mail]]]

Attachment: use GetPrincipal

Like this?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 244094 [details] [diff] [review]
use GetPrincipal

Looks great.  r+sr=bzbarsky

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Note, this is not needed for trunk, because NodePrincipal() is used there.

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 244094 [details] [diff] [review]
use GetPrincipal

approved for 1.8 branch, a=dveditz for drivers

Comment 9

[Steve England [:stevee]]

--> RESOLVED FIXED?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Yes, this should be fixed.

Comment 11

[Jay Patel [:jay]]

Does anyone have a simplified testcase or steps to reproduce this crash?  Would be nice to verify the fix...

Comment 12

[Boris Zbarsky [:bzbarsky]]

All we have is talkback data...