Closed Bug 611485 Opened 9 years ago Closed 9 years ago

Passing a Location object as a string argument of a cross-origin XPConnect call fails

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: james.kay, Assigned: mrbkap)

References

()

Details

(Keywords: regression, Whiteboard: [hardblocker] fixed-in-tracemonkey)

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_4; en-us) AppleWebKit/533.18.1 (KHTML, like Gecko) Version/5.0.2 Safari/533.18.5
Build Identifier: 4.0b7

Some fairly standard JavaScript to break out of a frameset does not work as intended.

 if (parent.frames.length > 0) {
        top.location.replace(document.location);
  }



Reproducible: Always

Steps to Reproduce:
1. Navigate to https://www.workbooks.com/login
2. Login with username: demo@workbooks.com   and password:  demo
3. Observe that the Desktop you see is framed; it should not be and is not in any other browser including in previous versions of Firefox.
Actual Results:  
The desktop is in a frame.

Expected Results:  
The desktop should occupy the whole browser window; the URL becomes https://secure.workbooks.com/ui/desktop

The website executes this JavaScript which appears to be ineffective:

 if (parent.frames.length > 0) {
        top.location.replace(document.location);
 }
Attached file testcase (obsolete) —
Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101108 Firefox/4.0b8pre

Testcase works fine on winXP, please test on Mac.
The testcase works for me too.

The steps to reproduce in comment 0 reproduce a problem for me, though.

The error console has this to say:

  Error: uncaught exception: [Exception... "Could not convert JavaScript
  argument arg 0 [nsIDOMLocation.replace]"  nsresult: "0x80570009
  (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame ::
  https://secure.workbooks.com/ui/desktop :: <TOP_LEVEL> :: line 16"  data: no]

The issue is that the argument to replace() needs to be a string, but document.location is an object...  Normally this would work by converting the object to a string, but in this case we land in XPCConvert::JSData2Native with the incoming jsval being a proxy.

So we call JS_ValueToString on this proxy.  This calls down into js_ValueToString, which calls js::DefaultValue, which calls js_TryMethod, which calls js_GetMethod.  This calls js::proxy_GetProperty with "toString" as the id.  

This ends up calling xpc::XrayWrapper<JSCrossCompartmentWrapper, xpc::CrossCompartmentXray>::get and passing it wrapper === receiver, which calls js::JSProxyHandler::get, passing it proxy === receiver.

Now we this last get() calls xpc::XrayWrapper<JSCrossCompartmentWrapper, xpc::CrossCompartmentXray>::getPropertyDescriptor.  We end up with desc->obj being null, call this->enter, get a false return, and return false all the way up the chain.  So the conversion fails.

The reason enter() fails is that xpc::CheckAndReport<xpc::SameOriginOrCrossOriginAccessiblePropertiesOnly> returns false (though it'd be nice if this were reported somewhere useful).

Blake, this sounds like your kettle of fish.  If you just breakpoint on the DOMString case in JSData2Native and condition the breakpoint on the incoming jsval_layout tag being OBJECT, then follow the steps to reproduce, you'll hit that breakpoint exactly for the replace() call in question.
Blocks: compartments
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Component: Location Bar → XPConnect
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
QA Contact: location.bar → xpconnect
Summary: top.location.replace behaviour change in beta7 → Passing a Location object as a string argument of a cross-origin XPConnect call fails
Ideally, this would just call the toString from nsIDOMLocation....

Even more ideally, it wouldn't look like the callee (window.top here) is the one trying to do the toString() call, since that's in fact something that shouldn't be allowed.
Andreas, is this related to the changes we took to refuse to pass certain types of C++ objects across origins, or is this something different?

Either way, assigning to mrbkap, and blocking.
Assignee: nobody → mrbkap
blocking2.0: ? → betaN+
Whiteboard: hardblocker
Whiteboard: hardblocker → [hardblocker]
Attached patch WIP (DOES NOT WORK) (obsolete) — Splinter Review
This is a work in progress. It doesn't actually succeed in calling functions through Xrays yet (we get a compartment mismatch trying to do so). I think it's pretty close, but the end result is going to be a least a little fragile. I think there might be a better way to do this by teaching XPConnect internals more about compartments. But for now, this approach *should* work.
Attachment #489992 - Attachment is obsolete: true
This patch mostly works, but is too incompatible to land. I'm working on an alternative that will be more compatible.
Attachment #505335 - Attachment is obsolete: true
Attached patch FixSplinter Review
Attachment #506562 - Flags: review?(gal)
Comment on attachment 506562 [details] [diff] [review]
Fix

We still have to rename CrossOriginWrappers. Nice to get rid of the Same/Cross compartment Xray business. This patch is definitely a step forward.
Attachment #506562 - Flags: review?(gal) → review+
http://hg.mozilla.org/tracemonkey/rev/a2825fbe23e3
Whiteboard: [hardblocker] → [hardblocker] fixed-in-tracemonkey
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110206 Firefox/4.0b12pre

Confirming issue as being fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.