Closed
Bug 300325
Opened 19 years ago
Closed 19 years ago
[FIX]Unable to set "expando" properties on an XPCNativeWrapper
Categories
(Core :: XPConnect, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.8beta4
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file, 2 obsolete files)
3.39 KB,
patch
|
jst
:
review+
brendan
:
superreview+
brendan
:
approval1.8b4+
|
Details | Diff | Splinter Review |
STEPS TO REPRODUCE: 1) Add the following code to the end of the initMenu method in browser.js: this.target.ownerDocument.documentElement.foo = 1; alert(this.target.ownerDocument.documentElement.foo); 2) Build in browser/, start Firefox, open a context menu ACTUAL RESULTS: Alert showing "undefined" EXPECTED RESULTS: Alert showing "1" I bet the problem is that the NewResolve hook for XPCNativeWrapper doesn't define the property if it's not an XPConnectified one... Is this really something we want not working with XPCNativeWrapper, though? In any case, this blocks bug 295937, since I can't test a patch to that bug because I can't set a property on an XPCNativeWrapper at all.
Assignee | ||
Updated•19 years ago
|
Assignee | ||
Comment 1•19 years ago
|
||
I thought this would work, but it didn't.... As a test, I tried changing the new code to: var test = this.target.ownerDocument.documentElement; test.foo = 1; alert(test.foo); but this has the same result -- "undefined"
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.8b4?
Comment 2•19 years ago
|
||
<bz> brendan: is there anything special that XPCNativeWrapper needs to do to allow random JS props to be set on it? <brendan> bz: this must be in the should *not* bypass case? <bz> brendan: yeah, this is the not bypass case <bz> brendan: I'm just chrome setting random expandos on content <brendan> in the !iface case <bz> yeah <brendan> and the !member case <brendan> just leave *vp alone <brendan> don't set it to JSVAL_VOID <brendan> in both places <brendan> easy peasy /be
Updated•19 years ago
|
Assignee: dbradley → bzbarsky
Flags: blocking1.8b4? → blocking1.8b4+
Assignee | ||
Comment 3•19 years ago
|
||
Attachment #188901 -
Attachment is obsolete: true
Attachment #188903 -
Flags: superreview?(brendan)
Attachment #188903 -
Flags: review?(jst)
Assignee | ||
Comment 4•19 years ago
|
||
Attachment #188903 -
Attachment is obsolete: true
Attachment #188904 -
Flags: superreview?(brendan)
Attachment #188904 -
Flags: review?(jst)
Assignee | ||
Updated•19 years ago
|
Attachment #188903 -
Attachment is obsolete: false
Attachment #188903 -
Flags: superreview?(brendan)
Attachment #188903 -
Flags: superreview-
Attachment #188903 -
Flags: review?(jst)
Attachment #188903 -
Flags: review-
Assignee | ||
Updated•19 years ago
|
Priority: -- → P1
Summary: Unable to set "expando" properties on an XPCNativeWrapper → [FIX]Unable to set "expando" properties on an XPCNativeWrapper
Target Milestone: --- → mozilla1.8beta4
Comment 5•19 years ago
|
||
Comment on attachment 188904 [details] [diff] [review] And the right diff What I said ;-). Sorry I didn't spot this earlier. /be
Attachment #188904 -
Flags: superreview?(brendan) → superreview+
Updated•19 years ago
|
Attachment #188903 -
Attachment is obsolete: true
Comment 6•19 years ago
|
||
So um, why do we even want to fix this bug? I'd rather see us throw an exception if code tries to set a non-existent property on a DOM node through a wrapper, you simply shouldn't do that, IMO.
Assignee | ||
Comment 7•19 years ago
|
||
The reason this came up is that people were asking me about wrapper lifetime. I assume that means they want to set something on the wrappers themselves... As part of trying to define this lifetime, we need to resolve bug 295937 one way or another, which leads us to this bug. I guess what I'm thinking about is extensions trying to set their own stuff on DOM nodes (eg what adblock does now unsafely) and being able to do so in a safe way. If we don't allow setting properties on an XPCNativeWrapper, they can't do that... irc conversation jst and I just had: <jst_> bz: I just asked why we would want to support that at all <jst_> IMO we shouldn't... > because for implicit wrappers, imo, we want to be as much like actual nodes as possible... <jst_> IMO no <jst_> even if they're implicit, they're still wrappers <jst_> they should reflect the DOM, and people should know that > I guess the question is what the pros and cons are.... > If extensions want to use expandos safely, at the moment they can't <jst_> if we start supporting adding properties, then we're back in a world where stuff can be hidden from our chrome, though not by content code, of course > right > But chrome hiding stuff from chrome might be needed... > perhaps <jst_> I'd want to see a strong demand for that before we'd support it <jst_> since we're starting down the same slippery slope <jst_> if we support that <jst_> even if the slope isn't nearly as steep > OK. > Fair enough... <jst_> so I'd say for now we should just throw an exception if code attempts to do this so they don't get inconsistent behavior <jst_> depending on when GC happens etc
Comment 8•19 years ago
|
||
Then I came on and argued a bit that in JS, most objects are expandable, so why not XPCNativeWrappers. Now we hear in bug 297543 from a real extension's authors about cases where the alternatives if this bug isn't fixed range from ugly to untenable. So I say we should fix this. The patch is trivial. jst, bz, anyone else with XPCNW standing: what do you say? /be
Comment 9•19 years ago
|
||
Comment on attachment 188904 [details] [diff] [review] And the right diff Fair enough. Though this is still pretty confusing to our developers, I bet this will cause us to get bugs about content not seeing properties set by chrome etc, more so than w/o this at least. Please do write this up as part of our XPCNativeWrapper docs ASAP. if (!member) { // No member, no property. - *vp = JSVAL_VOID; - That comment is not really true any more now then, maybe make it say something like "No member, no *IDL* property to expose." r=jst
Attachment #188904 -
Flags: review?(jst) → review+
Comment 10•19 years ago
|
||
Comment on attachment 188904 [details] [diff] [review] And the right diff We need this for 1.8/Fx1.1. /be
Attachment #188904 -
Flags: approval1.8b4+
Assignee | ||
Comment 11•19 years ago
|
||
Made the comment changes, and checked in. I've updated the documentation by adding the section at http://developer-test.mozilla.org/en/docs/XPCNativeWrapper#Setting_.22expando.22_properties_on_XPCNativeWrapper -- please let me know what you think, ok?
Assignee | ||
Comment 12•19 years ago
|
||
Fixed, even.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•