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

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P1)

Description

[Jesse Ruderman]

Attachment: 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.

Comment 1

[Brandon Sterne (:bsterne)]

Jesse, should this be sg:critical?

Comment 2

[Johnny Stenback (:jst)]

Yeah, this could be bad. Blocker, and over to Ben.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Worked through this with mrbkap the other day, we think this should be ok.

Comment 4

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

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();

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 6

[Johnny Stenback (:jst)]

Yeah, that sounds right.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1.1

Ok, how about this! Passes all mochitests.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 9

[Johnny Stenback (:jst)]

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.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed changeset 314b79846f1b to mozilla-central.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed changeset f0e3736f6b9a to mozilla-1.9.1.

Comment 12

[Aakash Desai [:aakashd]]

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