Closed Bug 623810 Opened 15 years ago Closed 14 years ago

document.write in an event handler crashes at [@ XPCWrappedNative::IsValid() ]

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: khuey, Assigned: mrbkap)

References

(
URL
)

Details

(Keywords: crash, regression, Whiteboard: [sg:critical][hardblocker][needs review mrbkap][has patch])

Crash Data

Attachments

(2 files, 4 obsolete files)

First part.
8.09 KB, patch
jst
: review+
Details | Diff | Splinter Review
Second part v2
7.32 KB, patch
Details | Diff | Splinter Review
Attached file testcase (WILL CRASH FIREFOX!) (obsolete) —
No description provided.
Attachment #501901 - Attachment description: testcase → testcase (WILL CRASH FIREFOX!)
Attached patch testcase (CRASHER) (obsolete) — — Splinter Review
Actually, the reload isn't even needed, we crash on the .write
Attachment #501901 - Attachment is obsolete: true
Just calling document.write from onload is enough. <body onload="document.write('');"> will do it.
Summary: Crash at [@ XPCWrappedNative::IsValid() ] → document.write in onload Crash at [@ XPCWrappedNative::IsValid() ]
Summary: document.write in onload Crash at [@ XPCWrappedNative::IsValid() ] → document.write in an event handler crashes at [@ XPCWrappedNative::IsValid() ]
This is a regression from Bug 606585.
Blocks: 606585
blocking2.0: --- → ?
Keywords: regressionwindow-wanted
Also this happens on Linux too.
OS: Windows 7 → All
Hardware: x86 → All
Hmm, I can't reproduce this on linux at all. Kyle, you still seeing this?
Yes, although it turns out it's not as simple as I thought. If you load data:text/html;charset=utf-8,<body onload%3D"document.write('')%3B"> you don't crash, but if you type <body onload="document.write('');"> into Hixie's data URI kitchen and go the crash appears. (Sometimes it took two or three tries.)
Keywords: testcase → testcase-wanted
On Mac I get a "compartment mismatch" following the steps in comment 6.
Whiteboard: [sg:critical]
Blocking, I can reproduce with str from comment 6 as well, and I see all sorts of wrong.
blocking2.0: ? → betaN+
Whiteboard: [sg:critical] → [sg:critical][hardblocker]
Attached patch "fix" (obsolete) — — Splinter Review
Blake, this "fixes" the issues seen here, but I'm not convinced all of this is right. I've seen the following problems by following the steps in comment 6: - Compartment mismatches in and under nsGlobalWindow::SetNewDocument() - Compartment mismatches when setting a wrapper wrapper's parent in XPCWrappedNative::ReparentWrapperIfFound(). My fix for this was to wrap the wrapper wrapper when reparenting. I hope there's something else we can do there... - Problems with reparenting a wrapper whose wanted parent isn't reparented to the new scope yet. I fixed this by making the wrapper reparenting recursively move the parent in that case. With this patch I'm unable to reproduce any compartment mismatches or other assertions here, but I have not run any other real tests yet.
Attachment #503883 - Flags: review?(mrbkap)
Assignee: nobody → jst
Whiteboard: [sg:critical][hardblocker] → [sg:critical][hardblocker][has patch][needs review mrbkap]
Attached patch First part. — — Splinter Review
This is the first and biggest part of the fix for this issue. mrbkap says r=mrbkap on this. The remaining part is to brain transplant the wrapper wrapper when moving a wrapper from one scope to another.
Attachment #506116 - Flags: review+
Attachment #503883 - Attachment is obsolete: true
Attachment #503883 - Flags: review?(mrbkap)
This needs transplanting code specific to the location object. mrbkap has a plan.
Assignee: jst → mrbkap
Whiteboard: [sg:critical][hardblocker][has patch][needs review mrbkap] → [sg:critical][hardblocker][needs review mrbkap]
jst checked the first part in as <http://hg.mozilla.org/mozilla-central/rev/fb8e4cdf2346>, for the record.
Attached patch Second part (obsolete) — — Splinter Review
I'm really unhappy about having to basically copy/paste JS_TransplantObject like this. But there are some very subtle differences about what wrappers/objects we use when. With this patch, the old location object ends up getting forgotten and we make the cross compartment wrapper be the old location wrapper. With this, expandos end up getting lost, but after talking this over with jst, we've decided that this is rare enough to not have to worry about for now.
Attachment #511192 - Flags: review?(jst)
Attachment #511192 - Flags: review?(gal)
Comment on attachment 511192 [details] [diff] [review] Second part - In XPCWrappedNative::ReparentWrapperIfFound(): + JSObject *ww = wrapper->GetWrapper(); + if(ww && xpc::WrapperFactory::IsLocationObject(flat)) + { + JSObject *newwrapper = xpc::WrapperFactory::WrapLocationObject(ccx, newobj); + if(!newwrapper) + return NS_ERROR_FAILURE; + + ww = js_TransplantLocation(ccx, flat, ww, newobj, newwrapper); + if(!ww) + return NS_ERROR_FAILURE; + flat = newobj; + wrapper->SetWrapper(ww); + } + else + { + flat = JS_TransplantObject(ccx, flat, newobj); + if(!flat) + return NS_ERROR_FAILURE; + } Don't we want to call wrapper->SetWrapper(null) in the else case there for the case where there's a wrapper wrapper, but it's not a location object wrapper? r=jst with that. The rest of the patch looks good to me, but I'm going to rely on gal to look at the details there.
Attachment #511192 - Flags: review?(jst) → review+
Whiteboard: [sg:critical][hardblocker][needs review mrbkap] → [sg:critical][hardblocker][needs review mrbkap][has patch]
Attachment #511192 - Flags: review?(gal) → review+
I compared this to the regular transplant path and it looks good. This is a high risk patch though :( I remember how many times we got the original path wrong and it took a while to catch all the bugs. How can we test this better?
Attached patch Second part v2 — — Splinter Review
Attachment #511192 - Attachment is obsolete: true
Oh, we need to rename js_TransplantLocation to js_TransplantObjectWithWrapper.
Second part landed: http://hg.mozilla.org/mozilla-central/rev/b5b177ec4d5d
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Looks like this got accidentally backed out in a tracemonkey merge.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Bumping this to final, we'll re-land this once the tree is open again. We already got 6 days worth of nightly testing on this w/o any fallout, which makes me feel more ok with shipping beta12 w/o this fix.
blocking2.0: betaN+ → final+
Can we find out how this was accidentally backed out? Thats really not good.
This isn't the first time that's happened recently either.
We decided to take this for beta12 either way. Re-landed as: http://hg.mozilla.org/mozilla-central/rev/42e7f9088975 Restoring betaN+ blocking status.
Status: REOPENED → RESOLVED
blocking2.0: final+ → betaN+
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
And yeah, we need to find out how this happened, and if there was anything other than this that accidentally got reverted.
Crash Signature: [@ XPCWrappedNative::IsValid() ]
Blake, Johnny, can we open this?
Yeah, I think so.
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: