Crashes [@ nsHTMLDocument::GetCookie]

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
--
critical
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({fixed1.8.1.1})

1.8 Branch
x86
All
fixed1.8.1.1
Points:
---
Bug Flags:
blocking1.8.1.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [need testcase])

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
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
(Assignee)

Comment 1

11 years ago
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?
(Assignee)

Comment 3

11 years ago
(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
(Assignee)

Comment 4

11 years ago
(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.
(Assignee)

Comment 5

11 years ago
Created attachment 244094 [details] [diff] [review]
use GetPrincipal

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+
(Assignee)

Updated

11 years ago
Attachment #244094 - Flags: approval1.8.1.1?
(Assignee)

Comment 7

11 years ago
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+
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1.1
--> RESOLVED FIXED?
(Assignee)

Comment 10

11 years ago
Yes, this should be fixed.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 11

11 years ago
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...

Updated

9 years ago
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.