Last Comment Bug 781476 - expando properties aren't accessible on certain objects when running same origin code in different compartment
: expando properties aren't accessible on certain objects when running same ori...
Status: RESOLVED FIXED
[qa-]
: regression
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: 15 Branch
: x86_64 Windows 7
: -- blocker with 14 votes (vote)
: mozilla17
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
: Ioana (away)
Mentors:
: 781441 (view as bug list)
Depends on: 787637
Blocks: cpg
  Show dependency treegraph
 
Reported: 2012-08-09 04:01 PDT by jmahesh
Modified: 2012-09-28 09:52 PDT (History)
19 users (show)
anthony.s.hughes: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
fixed
+
fixed


Attachments
test_ff15.rar (1.11 KB, application/x-rar)
2012-08-09 04:01 PDT, jmahesh
no flags Details
Test. v1 (2.48 KB, patch)
2012-08-10 02:22 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Workaround for Beta - Cross-compartment wrap events even if they don't have PreCreate. v1 (2.12 KB, patch)
2012-08-12 16:19 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Cross-compartment wrap same-origin events even if they don't have PreCreate. v2 (2.25 KB, patch)
2012-08-13 01:40 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Cross-compartment wrap same-origin objects with PreCreate even if PreCreate requests one wrapper per scope. v3 (1.49 KB, patch)
2012-08-14 08:44 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description jmahesh 2012-08-09 04:01:41 PDT
Created attachment 650499 [details]
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.
Comment 1 jmahesh 2012-08-09 04:04:52 PDT
Issue occurs only in Firefox15 , working fine in Firefox14, chrome, IE8 and safari.
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-09 08:13:36 PDT
*** Bug 781441 has been marked as a duplicate of this bug. ***
Comment 3 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-09 10:31:17 PDT
Sounds like something Bug 650353 could have caused.
Comment 4 Boris Zbarsky [:bz] 2012-08-09 11:12:02 PDT
Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2db9df42823d&tochange=0a48e6561534

Bug 650353 is indeed in that range.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-08-10 02:22:53 PDT
Created attachment 650833 [details] [diff] [review]
Test. v1

Added a proper mochitest.
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-08-10 02:24:20 PDT
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.
Comment 7 jmahesh 2012-08-10 02:28:25 PDT
INFO : As we are accessing library in a iframe, keep the attached test pages in the web server to reproduce the issue.
Comment 8 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-10 03:19:10 PDT
(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?
Comment 9 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-10 03:20:55 PDT
We need a fix for this for the beta, so converting events to use paris bindings isn't an option.
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2012-08-10 03:36:00 PDT
(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?
Comment 11 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-10 11:35:50 PDT
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?
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-08-10 15:37:02 PDT
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.
Comment 13 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-10 15:48:23 PDT
(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.
Comment 14 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-11 04:49:52 PDT
We should check what else doesn't have PreCreate.
Comment 15 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-11 04:50:57 PDT
...since cpg (bug 650353) isn't in anyway event specific.
Comment 16 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-11 04:58:27 PDT
Hmm, events do have nsIXPCScriptable::WANT_PRECREATE. But is the default PreCreate not enough?
Comment 17 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-11 06:52:41 PDT
The problem happens with many other objects too.
At least range, treewalker, various svg related objects, xsltprosessor, xpathevaluator, domparser...
This is bad.
Comment 18 Boris Zbarsky [:bz] 2012-08-12 09:09:19 PDT
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...
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2012-08-12 09:43:15 PDT
(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.
Comment 20 Bobby Holley (:bholley) (busy with Stylo) 2012-08-12 09:49:12 PDT
Can someone who knows more about events tell me what properties should be cross-origin accessible? Is this specified anywhere in HTML5 / WebIDL?
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-08-12 09:56:01 PDT
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.
Comment 22 Boris Zbarsky [:bz] 2012-08-12 10:24:53 PDT
> 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...
Comment 23 Boris Zbarsky [:bz] 2012-08-12 10:25:55 PDT
> 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....
Comment 24 Bobby Holley (:bholley) (busy with Stylo) 2012-08-12 11:46:46 PDT
(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?
Comment 25 Boris Zbarsky [:bz] 2012-08-12 13:24:27 PDT
> 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)?
Comment 26 Bobby Holley (:bholley) (busy with Stylo) 2012-08-12 13:44:28 PDT
(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.
Comment 27 Boris Zbarsky [:bz] 2012-08-12 13:52:58 PDT
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.
Comment 28 Bobby Holley (:bholley) (busy with Stylo) 2012-08-12 16:19:52 PDT
Created attachment 651238 [details] [diff] [review]
Workaround for Beta - Cross-compartment wrap events even if they don't have PreCreate. v1

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?
Comment 29 Andrew McCreight [:mccr8] 2012-08-12 16:31:17 PDT
(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.
Comment 30 Andrew McCreight [:mccr8] 2012-08-12 17:45:09 PDT
https://tbpl.mozilla.org/?tree=Try&rev=1ee0346c5ef3
Comment 31 Bobby Holley (:bholley) (busy with Stylo) 2012-08-13 01:40:25 PDT
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
Comment 32 Bobby Holley (:bholley) (busy with Stylo) 2012-08-13 01:40:45 PDT
Created attachment 651283 [details] [diff] [review]
Cross-compartment wrap same-origin events even if they don't have PreCreate. v2
Comment 33 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-13 03:17:37 PDT
(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 34 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-13 03:20:46 PDT
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.
Comment 35 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-13 03:23:26 PDT
(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.
Comment 36 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-13 03:26:25 PDT
Or are those tests exposing chrome stuff to content. That is of course possible cross-origin
access.
Comment 37 Boris Zbarsky [:bz] 2012-08-13 07:05:08 PDT
Those tests are using SpecialPowers to add chrome event listeners and whatnot....
Comment 38 Bobby Holley (:bholley) (busy with Stylo) 2012-08-13 15:42:12 PDT
(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.
Comment 39 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-13 16:29:46 PDT
Events and many other objects do have a PreCreate, nsDOMClassInfo::PreCreate.
I guess you mean some particular kind of PreCreate, or some other PreCreate :)
Comment 40 Bobby Holley (:bholley) (busy with Stylo) 2012-08-13 16:38:31 PDT
(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.
Comment 41 Bobby Holley (:bholley) (busy with Stylo) 2012-08-13 16:39:00 PDT
Here's a patch that hopefully works in all the cases we care about:

https://tbpl.mozilla.org/?tree=Try&rev=ef34794dee1b
Comment 42 jmahesh 2012-08-14 06:36:31 PDT
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.
Comment 43 Bobby Holley (:bholley) (busy with Stylo) 2012-08-14 08:44:31 PDT
Created attachment 651785 [details] [diff] [review]
Cross-compartment wrap same-origin objects with PreCreate even if PreCreate requests one wrapper per scope. v3

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.
Comment 44 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-14 08:51:43 PDT
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.
Comment 45 Bobby Holley (:bholley) (busy with Stylo) 2012-08-14 09:29:39 PDT
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 46 Blake Kaplan (:mrbkap) 2012-08-14 11:40:19 PDT
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).
Comment 47 Bobby Holley (:bholley) (busy with Stylo) 2012-08-14 11:44:41 PDT
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
Comment 48 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-14 11:50:08 PDT
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.
Comment 49 Bobby Holley (:bholley) (busy with Stylo) 2012-08-14 12:30:06 PDT
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.
Comment 50 Bobby Holley (:bholley) (busy with Stylo) 2012-08-14 12:31:23 PDT
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
Comment 51 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-14 13:29:41 PDT
We should land the patch to m-c and aurora too to get more testing (and to get less broken
behavior everywhere)
Comment 52 Ed Morley [:emorley] 2012-08-14 14:03:50 PDT
(In reply to Bobby Holley (:bholley) from comment #49)
> 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.

Sorry backed out of beta for m-oth failures:
https://tbpl.mozilla.org/?tree=Mozilla-Beta&rev=a0b80f837952
https://tbpl.mozilla.org/php/getParsedLog.php?id=14377133&tree=Mozilla-Beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=14377362&tree=Mozilla-Beta
(everything except the browser_pdfjs_main.js failures)

https://hg.mozilla.org/releases/mozilla-beta/rev/9226979297aa
Comment 53 Bobby Holley (:bholley) (busy with Stylo) 2012-08-14 15:42:28 PDT
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
Comment 54 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-14 16:33:07 PDT
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.)
Comment 55 Bobby Holley (:bholley) (busy with Stylo) 2012-08-14 16:38:51 PDT
(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.
Comment 56 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-14 16:43:34 PDT
Well, how difficult is it to implement PreCreate correctly, and what does 'correctly' mean in this case?
Comment 57 Bobby Holley (:bholley) (busy with Stylo) 2012-08-14 17:06:26 PDT
(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").
Comment 58 Boris Zbarsky [:bz] 2012-08-14 18:35:55 PDT
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).
Comment 59 anand 2012-08-16 04:02:21 PDT
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.
Comment 60 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-16 04:12:34 PDT
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.
Comment 61 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-16 04:15:39 PDT
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.
Comment 62 anand 2012-08-16 04:59:33 PDT
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.
Comment 63 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-08-16 05:05:24 PDT
(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
Comment 64 jmahesh 2012-08-16 05:18:45 PDT
(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.
Comment 65 Bobby Holley (:bholley) (busy with Stylo) 2012-08-16 12:26:38 PDT
(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 66 Bobby Holley (:bholley) (busy with Stylo) 2012-08-16 12:27:25 PDT
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.
Comment 67 jmahesh 2012-08-16 22:00:48 PDT
(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.
Comment 70 Ioana (away) 2012-08-28 04:42:37 PDT
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".
Comment 71 Ioana (away) 2012-09-28 04:40:47 PDT
This test is verified by a mochiest pushed with the fix, so there should be no need for further qa verifications.
Comment 72 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-28 09:52:50 PDT
(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+.

Note You need to log in before you can comment on or make changes to this bug.