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

RESOLVED FIXED

Status

()

Core
DOM
--
critical
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: khuey, Assigned: mrbkap)

Tracking

({crash, regression})

unspecified
crash, regression
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

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

Attachments

(2 attachments, 4 obsolete attachments)

Attachment #501901 - Attachment description: testcase → testcase (WILL CRASH FIREFOX!)
Created attachment 501902 [details] [diff] [review]
testcase (CRASHER)

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() ]
Keywords: crash, testcase
Keywords: regression, regressionwindow-wanted
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

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]
Created attachment 503883 [details] [diff] [review]
"fix"

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)

Updated

8 years ago
Assignee: nobody → jst

Updated

7 years ago
Whiteboard: [sg:critical][hardblocker] → [sg:critical][hardblocker][has patch][needs review mrbkap]
Created attachment 506116 [details] [diff] [review]
First part.

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+

Updated

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

Updated

7 years ago
Whiteboard: [sg:critical][hardblocker][has patch][needs review mrbkap] → [sg:critical][hardblocker][needs review mrbkap]
Created attachment 511192 [details] [diff] [review]
Second part

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+

Updated

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

Updated

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

Comment 15

7 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?
Created attachment 511873 [details] [diff] [review]
Second part v2
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
Last Resolved: 7 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+

Comment 21

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

http://hg.mozilla.org/mozilla-central/rev/42e7f9088975

Restoring betaN+ blocking status.
Status: REOPENED → RESOLVED
blocking2.0: final+ → betaN+
Last Resolved: 7 years ago7 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
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.