Closed Bug 781476 Opened 12 years ago Closed 12 years ago

expando properties aren't accessible on certain objects when running same origin code in different compartment

Categories

(Core :: XPConnect, defect)

15 Branch
x86_64
Windows 7
defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox15 + verified
firefox16 + fixed
firefox17 + fixed

People

(Reporter: jmahesh, Assigned: bholley)

References

Details

(Keywords: regression, Whiteboard: [qa-])

Attachments

(3 files, 2 obsolete files)

Attached file test_ff15.rar —
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.1 (KHTML, like Gecko) Chrome/21.0.1180.75 Safari/537.1

Steps to reproduce:

Steps to reproduce:

1)on body load , attaching library from a iframe.
2)calling method in iframe from the parent page.
3)In iframe library, attaching event for on mouseover of a div.
4)on mouseover, a property is set to event object and event handler is invoked.
5)In the event handler, properties set on the event object is accessed



Actual results:

Actual results:

The properties set on the event object are not accessible at the event handler.


Expected results:

1)The properties set on the event object should be accessible at the event handler.
Issue occurs only in Firefox15 , working fine in Firefox14, chrome, IE8 and safari.
Severity: normal → critical
Component: Untriaged → XPConnect
Product: Firefox → Core
Sounds like something Bug 650353 could have caused.
Attachment #650499 - Attachment mime type: text/plain → application/x-rar
Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2db9df42823d&tochange=0a48e6561534

Bug 650353 is indeed in that range.
Blocks: cpg
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Test. v1 — — Splinter Review
Added a proper mochitest.
So apparently, we don't for a parent in precreate for Events. this means that we get one wrapper per scope, so there's no way that expandos are going to be sharable across scopes.

The only way to fix this bug AFAICT is to fix events.
INFO : As we are accessing library in a iframe, keep the attached test pages in the web server to reproduce the issue.
(In reply to Bobby Holley (:bholley) from comment #6)
> The only way to fix this bug AFAICT is to fix events.
Meaning what? I guess you mean changing their binding?
We need a fix for this for the beta, so converting events to use paris bindings isn't an option.
(In reply to Olli Pettay [:smaug] from comment #8)
> Meaning what? I guess you mean changing their binding?

No, I mean implementing PreCreate and having one XPCWN per scope. Maybe peter knows why we don't yet do this?
We're two weeks away from 15 release and no one is taking this bug yet - can someone explain how serious this is in the wild? What does this look like on the web?  Does this affect embedded iframe content?
Keywords: regression
I think there's basically nothing we can do to fix this on beta. We'd need to change the way that Events are implemented in a way that's too risky (IMO) to land 2 weeks before shipping.

Somebody with more knowledge of events on the web would probably have a better sense of how bad it is.
(In reply to Bobby Holley (:bholley) from comment #12)
> I think there's basically nothing we can do to fix this on beta. We'd need
> to change the way that Events are implemented 
No changes to event implementation. Changes to the JS<->C++ binding of the events.

> Somebody with more knowledge of events on the web would probably have a
> better sense of how bad it is.
I don't really know how bad this is. Certainly bad, but is this critical...dunno.
We should check what else doesn't have PreCreate.
...since cpg (bug 650353) isn't in anyway event specific.
Hmm, events do have nsIXPCScriptable::WANT_PRECREATE. But is the default PreCreate not enough?
The problem happens with many other objects too.
At least range, treewalker, various svg related objects, xsltprosessor, xpathevaluator, domparser...
This is bad.
Severity: critical → blocker
Summary: properties on the event object is not set when the handlers are added through iframe → expando properties aren't accessible on certain objects when running same origin code in different compartment
We used to have separate scopes here before cpg, but got a wrapper that showed expandos, not a separate XPCWN, no?  Why are we suddenly getting a separate XPCWN now?  Can we revert that change?

As I see it, our options are:

1)  Fix this bug by reverting whatever part of CPG changed behavior here.
2)  Fix this bug by adding a PreCreate that sets a parent (the default one does not) for
    all DOM objects.
3)  Fix this bug by backing out CPG.
4)  Ship what looks like a serious web compat regression.

Option 1, if possible, looks simplest and safest...
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #18)
> We used to have separate scopes here before cpg, but got a wrapper that
> showed expandos, not a separate XPCWN, no?  Why are we suddenly getting a
> separate XPCWN now?  Can we revert that change?

"That change" would be CPG.

Since the initial days of compartments, we've always created a new XPCWN when wrapping something without a pre-create hook. This is because PreCreate is the only way for a native to tell us which scope it belongs to. If scope A and scope B independently access XPCOM object X, we don't have a good way to decide which scope the XPWN should end up in. For objects that implement nsWrapperCache, we could just go with whatever XPCWN was created first (as gross as that is). But it's less work to implement PreCreate than nsWrapperCache, and this still doesn't help us in the general case.

So anyway, this behavior is totally expected. I'd incorrectly assumed that we implemented PreCreate for anything where we allowed / cared about expandos. Indeed, for vanilla XPCOM XPCWNs we disallow expandos for this very reason. But it looks like this was incorrect. It's a shame it wasn't caught until now. :\

 
> As I see it, our options are:
> 1)  Fix this bug by reverting whatever part of CPG changed behavior here.

This is tantamount to option 3.

> 2)  Fix this bug by adding a PreCreate that sets a parent (the default one
> does not) for
>     all DOM objects.

This is possible. I'm pretty sure it would involve modifying our implementation of the same-origin policy, since my assumption is that we're supposed to be able to access all the properties on cross-origin events (that's what we implicitly do now, in any case). I have no idea whether it might break things though. I'll get cracking on this.

> 3)  Fix this bug by backing out CPG.

This is certainly possible, but it's risky. CPG landed near the beginning of the FF15 cycle (on May 4th - see bug 650353 comment 82), so there's a good chance that there are subsequent patches that either intentionally or unintentionally depend on it. It's a tractable issue, but requires some pretty thorough analysis.

> 4)  Ship what looks like a serious web compat regression.
Can someone who knows more about events tell me what properties should be cross-origin accessible? Is this specified anywhere in HTML5 / WebIDL?
Oh, also - wrapping events across compartments (as opposed to creating new XPCWNs) would mean that placing an expando on cross-origin events would fail (since XOWs don't allow expandos). Not sure if that would break stuff.
> If scope A and scope B

When you say "scope", do you mean global object, XPCWrappedNativeScope, compartment, or something else?

Because pre-cpg there was one compartment here, but two of everything else involved!  So how did it use to work?  I'd really like to understand that...
> Is this specified anywhere in HTML5

Yes.  HTML5 says all properties are accesible on all objects except Window, Document, and Location.  We've had that discussion before....
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #22)
> > If scope A and scope B
> 
> When you say "scope", do you mean global object, XPCWrappedNativeScope,
> compartment, or something else?

I mean the set of objects and scripts whose parent chain leads to a given global object. Before CPG, there was a one-to-one correspondence between "scopes", global objects, and XPCWrappedNativeScopes. After CPG that still holds, but there's also a one-to-one correspondence with the compartment.

> Because pre-cpg there was one compartment here, but two of everything else
> involved!  So how did it use to work?  I'd really like to understand that...

Right. So there was one XPCWN for the event (for a given origin / compartment). 
The object could be shared around between same-origin scopes without crossing a compartment boundary. It's only when we cross compartment boundaries that we need to wrap (and in this case, wrapping just creates a new XPCWN via WrapperFactory::PrepareForWrapping). And CPG caused us to have more compartment boundaries.

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #23)
> > Is this specified anywhere in HTML5
> 
> Yes.  HTML5 says all properties are accesible on all objects except Window,
> Document, and Location.  We've had that discussion before....

Ah, right. Thanks for reminding me. So the next question is whether it makes more sense to enumerate those properties in AccessCheck::IsPermitted, or whether we should just invent a new kind of unfiltered XOW that permits access to everything in IDL.

I assume we still need to permit cross-origin script access to events?
> It's only when we cross compartment boundaries that we need to wrap 

Ah, I see.

Hmm.  Is there no way we can make this compartment-crossing wrapping operation not create a new XPCWN, at least in some cases?  Presumably we have access to the old XPCWN there, right?

> I assume we still need to permit cross-origin script access to events?

That's not actually obvious to me.  How would a web page get its hands on a cross-origin event object (except chrome, which can obviously do that)?
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #25)
ompartment-crossing wrapping
> operation not create a new XPCWN, at least in some cases?  Presumably we
> have access to the old XPCWN there, right?

The issue is that we can encounter the same underlying XPCOM object through a different means (say, from an XPCOM getter), and without a wrapper cache we have no way of knowing where to look for the existing XPCWN during NativeInterface2JSObject. So we can end up with two objects corresponding to the same underlying native in a single compartment. This confuses identity checks and may violate various assumptions we make, but it might be ok for a stopgap solution. Worth a try.

> > I assume we still need to permit cross-origin script access to events?
> 
> That's not actually obvious to me.  How would a web page get its hands on a
> cross-origin event object (except chrome, which can obviously do that)?

I know very little about events. But if you think we might be able to get away with it, I'll just write the patch without modifying the same-origin policy and see if anything breaks.
Yes, I'm talking stopgap measure.  Obviously giving events reasonable parents is the right long-term solution; we need that for WebIDL bindings to work right.  But for now, I'm just after making the cases that already used to work work again for a bit in the least risky way possible.

> I know very little about events.

Talk to smaug?  He's a better resource here than I.
Attaching the workaround proposed in comment 26. It fixes the testcase, but nfortunately try is closed right now, so we can't check if it breaks anything else. I've got a flight to catch soon, so can someone push this to try when try re-opens?
(In reply to Bobby Holley (:bholley) from comment #28)
> I've got a flight to catch soon, so can someone push this to
> try when try re-opens?

Will do.
Looks like we do indeed rely in being able to access cross-origin events. I changed the patch to only attempt the workaround if we're wrapping same-origin.

https://tbpl.mozilla.org/?tree=Try&rev=84ef9a18b547
Attachment #651238 - Attachment is obsolete: true
(In reply to Bobby Holley (:bholley) from comment #24)
> 
> I assume we still need to permit cross-origin script access to events?
Only chrome can do cross-origin access to events.
Comment on attachment 651283 [details] [diff] [review]
Cross-compartment wrap same-origin events even if they don't have PreCreate. v2

Why special casing events? As far as I see, we need the same behavior for
many other objects too, probably for all which get the default
nsDOMClassInfo::PreCreate.
(In reply to Bobby Holley (:bholley) from comment #31)
> Looks like we do indeed rely in being able to access cross-origin events. 
Er, how? Just the same way other object can be used cross-origin.
Events aren't any special.
Or are those tests exposing chrome stuff to content. That is of course possible cross-origin
access.
Those tests are using SpecialPowers to add chrome event listeners and whatnot....
(In reply to Olli Pettay [:smaug] from comment #33)
> Only chrome can do cross-origin access to events.

In XPConnect parlance, chrome accessing content isn't considered cross-origin.

(In reply to Olli Pettay [:smaug] from comment #34)
> Why special casing events? As far as I see, we need the same behavior for
> many other objects too, probably for all which get the default
> nsDOMClassInfo::PreCreate.

Possibly, yes. I was trying to get a minimal green patch. Looks like the last one went green, so I can try to make it apply for anything that uses nsGenericSH.


(In reply to Olli Pettay [:smaug] from comment #35)
> (In reply to Bobby Holley (:bholley) from comment #31)
> Just the same way other object can be used cross-origin. Events aren't any special.

They're special compared to the common case for DOM objects in the sense that they don't implement PreCreate. This means that we get one XPCWN per scope, which effectively means that, if a page manages to get a reference to an event in another page, it has access to all properties instead of none of them.
Events and many other objects do have a PreCreate, nsDOMClassInfo::PreCreate.
I guess you mean some particular kind of PreCreate, or some other PreCreate :)
(In reply to Olli Pettay [:smaug] from comment #39)
> Events and many other objects do have a PreCreate, nsDOMClassInfo::PreCreate.
> I guess you mean some particular kind of PreCreate, or some other PreCreate
> :)

Er, right, sorry. I mean a PreCreate that actually does something useful. nsDOMClassInfo::PreCreate just uses whatever global was passed in.
Here's a patch that hopefully works in all the cases we care about:

https://tbpl.mozilla.org/?tree=Try&rev=ef34794dee1b
Indeed this is a blocker, and can impact many pages in the web. Have found this breaking in custom event's properties that are shared across iframes. 

Expecting the fix early in Firefox 15 Beta for a final check. 

Thanks for taking this up, it helps the web.
Ok, this is green on try. Flagging Blake for review. Note that I think we only want to land this on beta. I haven't tested it on beta yet (because we don't have useful try results for beta), but the code should be equivalent IIRC.
Attachment #651283 - Attachment is obsolete: true
Attachment #651785 - Flags: review?(mrbkap)
Beta 5 is going to build today, this is definitely the cutoff for speculative fixes so please prioritize the review and nomination of this patch as (depending on risk eval) it would need to land today.  Please include with the nomination a description of the impact this would have on webpages as well as how widespread this issue would be, if that can be estimated.
This is a risky patch, so we need Blake to review and make the call here. I just sent him an SMS, so hopefully he can weigh in here within the next hour or two.
Comment on attachment 651785 [details] [diff] [review]
Cross-compartment wrap same-origin objects with PreCreate even if PreCreate requests one wrapper per scope. v3

This is definitely the way to go in the short term: the original hack to create a new wrapper in these situations was based on the assumption that expandos weren't shared across compartments (or if they were, it was due to document.domain, and we had more hacks in place there to make that "work"). I'm a little worried about the identity check problem, since in practice those tend to be extremely difficult to debug -- however given the situation, I agree that we're less likely to have that problem (especially for events).
Attachment #651785 - Flags: review?(mrbkap) → review+
Comment on attachment 651785 [details] [diff] [review]
Cross-compartment wrap same-origin objects with PreCreate even if PreCreate requests one wrapper per scope. v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): CPG
User impact if declined: Potentially serious web compat regressions. We don't have a good sense of how widespread the issue is, but it breaks the event library of the reporter.
Testing completed (on m-c, etc.): Try only.
Risk to taking this patch (and alternatives if risky): This patch is risky, but we're pretty much up against the wall on this one. The two options are to land this fix, or to ship the regression. I think we should probably land the fix.
String or UUID changes made by this patch: none
Attachment #651785 - Flags: approval-mozilla-beta?
Comment on attachment 651785 [details] [diff] [review]
Cross-compartment wrap same-origin objects with PreCreate even if PreCreate requests one wrapper per scope. v3

Given the serious (and potentially large) breakage to web compatibility it's worth the risk to take this in Beta 5, we still have a chance to backout next week if this ends up being worse than not taking the fix in the first place.
Attachment #651785 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Pushed to m-b:
https://hg.mozilla.org/releases/mozilla-beta/rev/a0b80f837952

Still need to figure out what to do on m-c and m-a.
Note that the attached patch in this bug won't apply to m-b, because there are slight differences with m-c. For the diff as-pushed, see: https://hg.mozilla.org/releases/mozilla-beta/raw-rev/a0b80f837952
We should land the patch to m-c and aurora too to get more testing (and to get less broken
behavior everywhere)
Gah, sorry about that. This was a mismerge when merging the patch from m-c to m-b (where we don't have try coverage).

Basically, I replaced !isChrome(a) && subsumes(a,b) with isSameOrigin(a, b), since |subsumes| didn't exist on beta. I got rid of the isChrome(a) check, because the original intention of it was to make the subsumes check behave more like isSameOrigin (because chrome subsumes everything, meaning that chrome callers would otherwise fall into the workaround unconditionally).

But as it turns out, the isChrome(a) check was also necessary for another reason: we have browser-chrome tests that run into the identity check problem with objects from other chrome scopes.

So I'm adding the isChrome check back in, and continuing to hope that the web doesn't rely on these kinds of identity comparisons as much as our browser chrome does:

https://hg.mozilla.org/releases/mozilla-beta/rev/d9e51958517a
Just curious, how difficult would it be to fix identity comparisons?
(Since I think we'll need to fix those in Aurora or trunk - we won't have WebIDL bindings for 
all the web exposed objects any time soon.)
(In reply to Olli Pettay [:smaug] from comment #54)
> Just curious, how difficult would it be to fix identity comparisons?

Not possible. Getting identity comparisons right is the whole reason we create a new XPCWN per compartment when PreCreate requests one wrapper-per-scope.

> (Since I think we'll need to fix those in Aurora or trunk - we won't have
> WebIDL bindings for 
> all the web exposed objects any time soon.)

The correct solution for those branches is to implement PreCreate properly.
Well, how difficult is it to implement PreCreate correctly, and what does 'correctly' mean in this case?
(In reply to Olli Pettay [:smaug] from comment #56)
> Well, how difficult is it to implement PreCreate correctly, and what does
> 'correctly' mean in this case?

(We're discussing this on IRC. But for posterity, "correctly" means "Precreate returns the same parent regardless of what globalObj is passed").
I don't think a patch to change PreCreate behavior will fly for Aurora.  It _might_ fly for trunk.  Maybe.  It's late in the cycle, so it's a bit risky.

I think our path of least risk is to land the patch that landed on beta on m-c and aurora, like smaug said, then focus on giving everything a parent.  This will be a prereq for WebIDL bindings anyway, but can be done without any of the WebIDL binding stuff (and in particular, doesn't involve doing anything with wrapper caches).
Can you please confirm the availability of this fix in FireFox 15 ?. 

Please note, we need this fix in FireFox 15, as  many  of our enterprise customers are using our product in FireFox, and they would be affected seriously by this defect.
Based on comment 53 the patch was pushed to beta, so I guess 
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/15.0b5-candidates/ should have the patch.
Please test.
Boddy, any reason you haven't pushed this to aurora and m-c? We need all the testing we can get, 
and this needs to be fixed in aurora and m-c too.
Thanks this particular issues is found working now. 

But I hear another break (which worked in Firefox 14), thought that would also be solved by this, but the problem still persist.

The problem is, 
1.Create a custom event.
2. Make and Set a property with name returnValue to true
3. Fire so that it goes to the event handler, and when it comes back from the event handler, the value of returnValue is false, while the event handler did not change this property value at all.

This happens in certain scenario and when the property name is 'returnValue'

This possibly gives you hint already about the problem, while the technical team of jmahesh@cordys.com trying get a test page ready.


Worried if this also a Break ! Does it ring any alarm bell ?

(In reply to Olli Pettay [:smaug] from comment #60)
> Based on comment 53 the patch was pushed to beta, so I guess 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/15.0b5-candidates/
> should have the patch.
> Please test.
(In reply to anand from comment #62)
> The problem is, 
a minimal testcase would be great

> 1.Create a custom event.
Do you mean var ce = new CustomEvent("foo");?
or something else?


> 2. Make and Set a property with name returnValue to true
> 3. Fire so that it goes to the event handler, and when it comes back from
> the event handler, the value of returnValue is false, while the event
> handler did not change this property value at all.
> 
> This happens in certain scenario and when the property name is 'returnValue'
Are you using beforeUnloadEvent ?
That is the only event interface which has returnValue

> 
> Worried if this also a Break ! Does it ring any alarm bell ?
Don't know what might have caused that issue.
Please file a new bug and if possible, attach a minimal testcase.
CC me and bholley
(In reply to Olli Pettay [:smaug] from comment #63)
> (In reply to anand from comment #62)
> > The problem is, 
> a minimal testcase would be great
> 
> > 1.Create a custom event.
> Do you mean var ce = new CustomEvent("foo");?
> or something else?
> 
> 
We create a custom Object, and use our event handling mechanism for our CustomeEvent. So it is new Object.

> > 2. Make and Set a property with name returnValue to true
> > 3. Fire so that it goes to the event handler, and when it comes back from
> > the event handler, the value of returnValue is false, while the event
> > handler did not change this property value at all.
> > 
> > This happens in certain scenario and when the property name is 'returnValue'
> Are you using beforeUnloadEvent ?
No, it is pretty much in standard condition of the page which not closing.
Just that an event like said above (an object) is created and passed with this property name 
> That is the only event interface which has returnValue
> 
> > 
> > Worried if this also a Break ! Does it ring any alarm bell ?
> Don't know what might have caused that issue.
> Please file a new bug and if possible, attach a minimal testcase.
> CC me and bholley

We are trying to debug with Venkman to see if we can get a testpage.
(In reply to Olli Pettay [:smaug] from comment #61)
> Boddy, any reason you haven't pushed this to aurora and m-c? We need all the
> testing we can get, 
> and this needs to be fixed in aurora and m-c too.

m-i was closed when I was going to push it. Just pushed to m-i:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/87fb202e7261
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd865cc52c5
Comment on attachment 651785 [details] [diff] [review]
Cross-compartment wrap same-origin objects with PreCreate even if PreCreate requests one wrapper per scope. v3

We've already landed this hack on beta and m-i, so it makes sense to land it on m-a as well (for maximal test coverage, fixing the bug in the interim, and consistency). Flagging for m-a approval.
Attachment #651785 - Flags: approval-mozilla-aurora?
(In reply to Olli Pettay [:smaug] from comment #63)
> (In reply to anand from comment #62)
> > The problem is, 
> a minimal testcase would be great
> 
> > 1.Create a custom event.
> Do you mean var ce = new CustomEvent("foo");?
> or something else?
> 
> 
> > 2. Make and Set a property with name returnValue to true
> > 3. Fire so that it goes to the event handler, and when it comes back from
> > the event handler, the value of returnValue is false, while the event
> > handler did not change this property value at all.
> > 
> > This happens in certain scenario and when the property name is 'returnValue'
> Are you using beforeUnloadEvent ?
> That is the only event interface which has returnValue
> 
> > 
> > Worried if this also a Break ! Does it ring any alarm bell ?
> Don't know what might have caused that issue.
> Please file a new bug and if possible, attach a minimal testcase.
> CC me and bholley

The second issue is found to be caused because of the clash of HTML5 reserved word 'toolbar' and application's id. This property of HTML5 is being supported in Firefox15. We handled it in our layer.

The main event issue is fixed with this patch, we have tested and found it working fine. 
Thanks ,Waiting for next public Beta build to have this.
https://hg.mozilla.org/mozilla-central/rev/87fb202e7261
https://hg.mozilla.org/mozilla-central/rev/7bd865cc52c5
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Attachment #651785 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0 (20120824154833)

The alert displayed when applying the STR in comment 0 on the test page attached in the same comment, you now get: "eventObject._property is --cordys".
QA Contact: ioana.budnar
Depends on: 787637
Assignee: nobody → bobbyholley+bmo
Depends on: 789713
No longer depends on: 789713
Keywords: verifyme
This test is verified by a mochiest pushed with the fix, so there should be no need for further qa verifications.
Keywords: verifyme
Whiteboard: [qa-]
(In reply to Ioana Budnar [QA] from comment #71)
> This test is verified by a mochiest pushed with the fix, so there should be
> no need for further qa verifications.

Agreed, given that I'm marking this in-testsuite+.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.