Closed Bug 757639 Opened 7 years ago Closed 7 years ago

"expando" properties aren't accessible by chrome code in a module or component

Categories

(Core :: XPConnect, defect)

15 Branch
x86
Windows XP
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 758415

People

(Reporter: tabnotifier-mozilla, Assigned: bholley)

References

Details

(Keywords: addon-compat, regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/15.0 Firefox/15.0a1
Build ID: 20120522080220

Steps to reproduce:

Tried with Firefox 15.0a1 or SeaMonkey 2.12a1
Steps to reproduce it
1. Download and install the testcase extension
2. Right-click on an element of a web page (not "about:")
3. Select "Test the expandos bug" from the context menu

What this extension does after these steps:
1. Sets an "expando" property of the HTMLElement object
2. Passes the object to a function in a module




Actual results:

An alert box with the text below is showed:

Expando property value in the module: "undefined"
Expando property value in the original scope: "just a value"

(Some wrapper hides the expando property to the module)
The same result was obtained passing an HTMLDocument/any HTMLElement object to a method of a js XPCOM component


Expected results:

An alert box with the text below should have been showed:

Expando property value in the module: "just a value"
Expando property value in the original scope: "just a value"

(As it is with previous versions of the browsers)
This has compartment-per-global written all over it.
Component: General → XPConnect
QA Contact: general → xpconnect
Indeed, this is expected behavior. Maybe this will clear things up? http://bholley.wordpress.com/2012/05/04/at-long-last-compartment-per-global/
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Wait.  Why is this expected behavior, exactly?  Shouldn't same-origin cross-compartment wrappers show expandos?
(In reply to Boris Zbarsky (:bz) from comment #3)
> Wait.  Why is this expected behavior, exactly?  Shouldn't same-origin
> cross-compartment wrappers show expandos?

Presumably, |gContextMenu.target| (the object on which the expandos are placed) is a content DOM object (I also gathered this from STR step 2). So the two chrome scopes each get a separate Xray wrapper to the content object, and their expandos live on that Xray wrapper. As such, they can't be shared.
That seems like a really surprising consequence of CPG, and one that is likely to break extensions, imo.

Still imo, we should be sharing Xrays across chrome stuff or making this work in some other way.  The current setup is totally nonintuitive, because compartment boundaries are so arbitrary, and nothing like the setup content sees.  It's also a regression from CPG, for all intents and purposes.
Blocks: cpg
Status: RESOLVED → REOPENED
Ever confirmed: true
Keywords: regression
Resolution: INVALID → ---
(In reply to Boris Zbarsky (:bz) from comment #5)
> That seems like a really surprising consequence of CPG, and one that is
> likely to break extensions, imo.

Which is why I made such a big deal about it on my blog post.

> Still imo, we should be sharing Xrays across chrome stuff or making this
> work in some other way.

There's really no great way to do this that I can think of. This kind of expando sharing is incredibly painful - it's bad enough for Location (which we have to support to avoid breaking the web). Given that this is only visible from chrome code, we made the decision to just do it.

Peter, Blake? What are your thoughts? Boris, do you have any ideas on how we might go about sharing them?
> Which is why I made such a big deal about it on my blog post.

With all due respect... _I_ didn't understand that this was a consequence of the change, from your blog post.  I doubt any extension authors who read it (which was probably very few of them) did either.

The worst part is that, again, from the point of view of extension authors compartment boundaries are completely arbitrary.  So now they have no way to know whether passing an object to the function will allow the function to see the expandos they just set on the variable.  There's just no sane way to tell.  It makes using expandos pretty much impossible.

Perhaps passing an Xray from one chrome compartment to the other should just create a cross-compartment wrapper around it, not a new Xray?  There would still be weirdness if different chrome compartments independently poke the DOM, because then they would get different Xrays.  But at least the simple case of "Pass this exact object to a function" would not break...
(In reply to Boris Zbarsky (:bz) from comment #7)
> > Which is why I made such a big deal about it on my blog post.
> 
> With all due respect... _I_ didn't understand that this was a consequence of
> the change, from your blog post.  I doubt any extension authors who read it
> (which was probably very few of them) did either.

Fair enough. However clear it was from the blog post, I totally buy that the people who are going to get bit here aren't reading planet.

> The worst part is that, again, from the point of view of extension authors
> compartment boundaries are completely arbitrary. So now they have no way to
> know whether passing an object to the function will allow the function to
> see the expandos they just set on the variable.There's just no sane way to
> tell.

Sort of. It's per-global, which can be tricky at times but in general isn't completely arbitrary.

> It makes using expandos pretty much impossible.

I wouldn't go that far. I think it's pretty common for the usage of a single expando to be pretty contained in JSMs and sandboxes (like jetpack), at which point this issue doesn't appear. Indeed, the fact that we haven't seen more major breakage from the big addons is comforting. Lightning, for example, ran into trouble because they were passing e4x objects between scopes (which broke with CPG, because we don't support wrapping e4x objects), but AFAIK they didn't have any problems with expandos. Same for ABP I think. But you know way more than I do on this topic.
 
> Perhaps passing an Xray from one chrome compartment to the other should just
> create a cross-compartment wrapper around it, not a new Xray?  There would
> still be weirdness if different chrome compartments independently poke the
> DOM, because then they would get different Xrays.  But at least the simple
> case of "Pass this exact object to a function" would not break...

That's been bad news when we've tried it in the past. It confuses identity checks immensely, for the reasons you mentioned. It's quite confusing to have two references to the same DOM object not compare ===.

The only real solution that I can think of is a per-origin Xray expando map. We could also use this for Location objects, which would be awesome (and avoid the crappy double-wrapping we use to avoid making this problem visible to content in the Location case). But the logistics of it are pretty non-trivial. I'm certainly open to the idea though. What do you think, Blake?
> but in general isn't completely arbitrary.

I said "from the point of view of extension authors".  Does loading a subscript create a new global?  What about a JS component?  A module?  An overlay?  If you have to go read the code to answer the question, it's arbitrary from the point of view of extension authors...

> I think it's pretty common for the usage of a single expando to be pretty contained in
> JSMs and sandboxes 

Sure.  If you have a well-defined scope like that, it all works.  As soon as you try to interact with it from outside, though, you lose.

> The only real solution that I can think of is a per-origin Xray expando map.

If we wanted to, we could perhaps specialize this to chrome.  As in, for the system-principal xrays, hang the expando object off the underlying DOM object, not off the Xray.  I guess that wouldn't help with Location, though.
(In reply to Boris Zbarsky (:bz) from comment #9)
> If we wanted to, we could perhaps specialize this to chrome.  As in, for the
> system-principal xrays, hang the expando object off the underlying DOM
> object, not off the Xray.  I guess that wouldn't help with Location, though.

That seems like a reasonable solution. Though it relies on the Xray refactor/unification that I've been meaning to do but haven't done (because currently the expando object and the holder are the same for some types of Xrays but not others).

Blake, Peter? Thoughts?
(In reply to Boris Zbarsky (:bz) from comment #5)
> That seems like a really surprising consequence of CPG, and one that is
> likely to break extensions, imo.
> 
> Still imo, we should be sharing Xrays across chrome stuff or making this
> work in some other way.  The current setup is totally nonintuitive, because
> compartment boundaries are so arbitrary, and nothing like the setup content
> sees.  It's also a regression from CPG, for all intents and purposes.

If this was left unfixed, would there be user impact for existing add-ons, or is this a matter of correctness for the future?
There can be user impact for existing add-ons, depending on their exact code patterns.  I wouldn't be worrying about this as much if it were only about future stuff.
Depends on: 758415
Side note: I suffered breakage in FlashGot, noticed and fixed in less than 6 hours on May the 12th in 1.4.5rc1, by changing my code (which used to read from a XPCOM service an expando set by a chrome overlay) to a saner pattern which had been already adopted by NoScript for a long time: centralizing expando access, either in a component or in a module, with an interface like:

var DOMVar = {
  set: function(domObject, varName, varValue) { ... },
  get: function(domObject, varName, defaultValue) { ... },
  has: function(domObject, varName) { ... },
  // optionally an enumeration method could be added
}

I originally did it in NoScript to better namespacing (in respect of other add-ons) and controlling life cycle of my private expandos, but after this change (which makes namespacing less or not useful) a welcome side effect has been breakage prevention.

If bug 758415 proves to bee too hard/inconvenient to fix, this may be advertised as a good practice to add-ons developers.
I see Bobby is working on blocking bug 758415, so assigning to him. Jorge expressed his preference that this land in FF15 while it's on m-c or early in m-a at the latest for sake of add-on compat.
Assignee: nobody → bobby
Assignee: bobby → bobbyholley+bmo
At this point, this is just a duplicate of bug 758415.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 758415
No point in tracking both this and bug 758415. That bug landed.
You need to log in before you can comment on or make changes to this bug.