Closed Bug 467210 Opened 12 years ago Closed 12 years ago

"ASSERTION: PostCreate failed" playing with previous document in <iframe>

Categories

(Core :: XPConnect, defect, P1)

x86
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: jruderman, Assigned: bent.mozilla)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, verified1.9.1, Whiteboard: [sg:critical?])

Attachments

(2 files, 1 obsolete file)

Attached file testcase
Loading the testcase locally triggers:

###!!! ASSERTION: PostCreate failed! This is known to cause inconsistent state for some class types and may even cause a crash in combination with a JS GC. Fix the failing PostCreate ASAP!: 'Error', 
file js/src/xpconnect/src/xpcwrappednative.cpp, line 560

Based on the scariness of the assertion, I'm filing this bug as security-sensitive.
Summary: "ASSERTION: PostCreate failed" playing with document from removed frame → "ASSERTION: PostCreate failed" playing with previous document in <iframe>
Jesse, should this be sg:critical?
Yeah, this could be bad. Blocker, and over to Ben.
Assignee: nobody → bent.mozilla
Flags: blocking1.9.1+
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1b3
Whiteboard: [sg:critical?]
Attached patch Patch, v1 (obsolete) — Splinter Review
Worked through this with mrbkap the other day, we think this should be ok.
Attachment #352349 - Flags: superreview?
Attachment #352349 - Flags: review?(mrbkap)
Attachment #352349 - Flags: superreview? → superreview?(jst)
Comment on attachment 352349 [details] [diff] [review]
Patch, v1

Did you run all the relevant tests without getting assertions?

AFAIK, aOwner can be outer window, for example if XHR is created this way:
new (new XPCNativeWrapper(window)).XMLHttpRequest();
Oops, I missed that yesterday. Yeah, we need to bail if aOwner is an outer window and we can't access the current inner window.
Attachment #352349 - Flags: superreview?(jst)
Attachment #352349 - Flags: review?(mrbkap)
Attachment #352349 - Flags: review-
Yeah, that sounds right.
Attached patch Patch, v1.1Splinter Review
Ok, how about this! Passes all mochitests.
Attachment #353131 - Flags: superreview?(jst)
Attachment #353131 - Flags: review?(mrbkap)
Attachment #352349 - Attachment is obsolete: true
Comment on attachment 353131 [details] [diff] [review]
Patch, v1.1

smaug's testcase in comment 4 still doesn't work, but that's a much older regression (apparently) that should be filed and fixed separately. This logic seems terribly over complicated. I feel like someone else should be doing a GetCurrentInnerWindow() somewhere.
Attachment #353131 - Flags: review?(mrbkap) → review+
Attachment #353131 - Flags: superreview?(jst) → superreview+
Comment on attachment 353131 [details] [diff] [review]
Patch, v1.1

Yeah, I agree with mrbkap, this should be less complex and the inner/outer decision should be pushed out to the callers of this. Please file a followup bug to clean this up.

sr=jst for this for now.
Pushed changeset 314b79846f1b to mozilla-central.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Pushed changeset f0e3736f6b9a to mozilla-1.9.1.
Keywords: fixed1.9.1
Depends on: 471560
verified FIXED (no assertion via the attached testcase) on debug builds:


Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090608 Shiretoko/3.5pre ID:20090608122057

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090608 Minefield/3.6a1pre ID:20090608122028
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.