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




8 years ago
a month ago


(Reporter: khuey, Assigned: mrbkap)


({crash, regression})


Firefox Tracking Flags

(blocking2.0 betaN+)


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


(2 attachments, 4 obsolete attachments)

Attachment #501901 - Attachment description: testcase → testcase (WILL CRASH FIREFOX!)
Posted 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: --- → ?
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.)

Comment 7

8 years ago
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]
Posted 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
- 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


8 years ago
Whiteboard: [sg:critical][hardblocker] → [sg:critical][hardblocker][has patch][needs review mrbkap]
Posted 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


8 years ago
Whiteboard: [sg:critical][hardblocker][has patch][needs review mrbkap] → [sg:critical][hardblocker][needs review mrbkap]
jst checked the first part in as <>, for the record.
Posted 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+


8 years ago
Whiteboard: [sg:critical][hardblocker][needs review mrbkap] → [sg:critical][hardblocker][needs review mrbkap][has patch]


8 years ago
Attachment #511192 - Flags: review?(gal) → review+

Comment 15

8 years ago
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?
Attachment #511192 - Attachment is obsolete: true
Oh, we need to rename js_TransplantLocation to js_TransplantObjectWithWrapper.
Second part landed:
Last Resolved: 8 years ago
Resolution: --- → FIXED
Looks like this got accidentally backed out in a tracemonkey merge.
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+

Comment 21

8 years ago
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:

Restoring betaN+ blocking status.
blocking2.0: final+ → betaN+
Last Resolved: 8 years ago8 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
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.