Closed
Bug 842372
Opened 11 years ago
Closed 11 years ago
Make getUserData and setUserData ChromeOnly
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: emk, Assigned: emk)
References
Details
(Keywords: addon-compat, dev-doc-complete, site-compat)
Attachments
(2 files, 5 obsolete files)
2.73 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
7.70 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
test_bug548463.html essentially requires get/setUserData() as long as it's supported. So I've SpecialPower.wrap()ed the node to have an access to get/setUserData(). test_treewalker_nextsibling.xml used get/setUserData() only to test the object identity. So I've rewritten it using WeakMap. test_bug384632.html did no longer test what is supposed to be tested because Node migrated to WebIDL. Given that all DOM interfaces would migrate to WebIDL sooner or later, I've rewritten the test using nsIWritablePropertyBag.
Attachment #715218 -
Flags: review?(jonas)
Assignee | ||
Updated•11 years ago
|
Keywords: addon-compat,
dev-doc-needed
Assignee | ||
Comment 1•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5ef9fdc9d43f
![]() |
||
Comment 2•11 years ago
|
||
Doesn't this break addons that use get/setUserData from XBL bindings, if any?
Assignee | ||
Comment 3•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #2) > Doesn't this break addons that use get/setUserData from XBL bindings, if any? Yes, it breaks. Is it possible to add an extended attribute something like [ChromeOrXBLOnly]?
![]() |
||
Comment 4•11 years ago
|
||
Hmm. Once Bobby flips the separate-scope pref, won't XBL always see content through Xrays? If so, it'll automatically get treated as chrome for [ChromeOnly] purposes, I'd think.
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #4) > Hmm. > > Once Bobby flips the separate-scope pref, won't XBL always see content > through Xrays? If so, it'll automatically get treated as chrome for > [ChromeOnly] purposes, I'd think. Does it mean bug 834697? It is already fixed and I confirmed "dom.xbl_scopes" was enabled on my local build.
![]() |
||
Comment 6•11 years ago
|
||
Hmm. Yes, that's teh one I was thinking of. Bobby, any idea why this doesn't work with the pref flipped?
Assignee | ||
Comment 7•11 years ago
|
||
I think if ChromeOnly properties are visible from the content XBL, it is a security hole because content easily bind a XBL as my mochitest shown. At least I closed bug 814254 assuming that ChromeOnly properties are invisible from the content.
![]() |
||
Comment 8•11 years ago
|
||
Mochitests flip a pref that allows random XBL. Normal content can only apply chrome:// bindings, and even then only script inside the binding would be able to read [ChromeOnly] properties. Assuming that it works at all, of course... I'm still trying to understand why it doesn't, before we try to make it work for this case.
Assignee | ||
Comment 9•11 years ago
|
||
Do XBL compartments have the system principal? https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/AccessCheck.cpp?rev=1c2e7ae47afc#90
Comment 10•11 years ago
|
||
No.
Assignee | ||
Comment 11•11 years ago
|
||
Then it would be expected that XBLs would not see ChromeOnly properties. https://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py?rev=046df99b88b0#1503 https://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py?rev=046df99b88b0#1693
![]() |
||
Comment 12•11 years ago
|
||
> Do XBL compartments have the system principal? No, but I thought WebIDL Xrays always got the "chrome" view. And I thought that XBL compartments got the Xray view. Which one of those assumptions is wrong? The code quoted in comment 11 is for direct prototype setup, not Xrays.
Comment 13•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #12) > > Do XBL compartments have the system principal? > > No, but I thought WebIDL Xrays always got the "chrome" view. I sure hope not. After all, content can always get an Xray view to its own objects via Components.lookupMethod, so that would be a major security bug if that were the case. Marking this comment private until someone confirms that's not the case. > And I thought > that XBL compartments got the Xray view. Yes, on account of having nsExpandedPrincipal. > Which one of those assumptions is wrong? I sure hope the former.
![]() |
||
Comment 14•11 years ago
|
||
OK, it's the former. XrayResolveNativeProperty only looks at nativeProperties.chromeOnly when xpc::AccessCheck::isChrome(js::GetObjectCompartment(wrapper)). Unmarking comment 13 private bit. So given that, I suppose we could add another level of access that's always available Xrays, even unprivileged ones... Or make it XBL-specific somehow. Or something.
Comment 13 is private:
false
Assignee | ||
Comment 15•11 years ago
|
||
Will [Func="IsCallerChromeOrXBL"] work once bug 838691 has been landed?
![]() |
||
Comment 16•11 years ago
|
||
Oh, very nice. Assuming we can tell from the cx (presumably from it's compartment) that we're "chrome or xbl", then yes, I think it would.
Assignee | ||
Comment 17•11 years ago
|
||
Assignee: nobody → VYV03354
Attachment #715218 -
Attachment is obsolete: true
Attachment #715661 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #715218 -
Flags: review?(jonas)
Attachment #716269 -
Flags: review?(jonas)
Attachment #716269 -
Flags: review?(bzbarsky)
![]() |
||
Comment 18•11 years ago
|
||
Comment on attachment 716269 [details] [diff] [review] Hide getUserData and setUserData from content Very nice!
Attachment #716269 -
Flags: review?(bzbarsky) → review+
Comment 19•11 years ago
|
||
Given that this depends on XBL scopes, we'll have to back it out if we ever flip off dom.xbl_scopes. Marking the dep so that we make sure to remember.
Depends on: 834697
![]() |
||
Comment 20•11 years ago
|
||
We can just check the pref in the function, and claim the properties are enabled if the pref is flipped off, no? That might be a good idea.
Comment 21•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #20) > We can just check the pref in the function, and claim the properties are > enabled if the pref is flipped off, no? That might be a good idea. Yes, let's do that.
Assignee | ||
Comment 22•11 years ago
|
||
Attachment #716269 -
Attachment is obsolete: true
Attachment #716269 -
Flags: review?(jonas)
Attachment #717066 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Comment 23•11 years ago
|
||
Attachment #717067 -
Flags: review?(jonas)
Comment 24•11 years ago
|
||
Comment on attachment 717066 [details] [diff] [review] Part 1: Implement nsINode::IsChromeOrXBL Review of attachment 717066 [details] [diff] [review]: ----------------------------------------------------------------- I have some concerns with this patch: * Why do we take an unused JSObject parameter? Is it some artifact of what the CodeGen needs? * Why is this a function on nsINode? CodeGen again? * The pref-disabled behavior here is dangerous given what the function is named. I understand that it's ok for this use-case to just say "true" if xbl scopes are disabled, but if anyone else starts using it it might not be. I'd prefer to give this a very specific name like "ShouldExposeGetUserData" or something.
Attachment #717066 -
Flags: review?(bobbyholley+bmo) → review-
![]() |
||
Comment 25•11 years ago
|
||
> Is it some artifact of what the CodeGen needs? Yes. The codegen passes in the JSContext and the JSObject that the prop will be defined on so that callees can introspect those as needed. > * Why is this a function on nsINode? CodeGen again? It just needs to be a static function somewhere, for codegen purposes.
Assignee | ||
Comment 26•11 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #24) > * Why do we take an unused JSObject parameter? Is it some artifact of what > the CodeGen needs? Yes, the function signature must be something like |FuncName(JSContext*, JSOnject*)|. > * Why is this a function on nsINode? CodeGen again? I put it on nsINode because it is not a general-puopose function as you pointed out in the next bullet. > * The pref-disabled behavior here is dangerous given what the function is > named. I understand that it's ok for this use-case to just say "true" if xbl > scopes are disabled, but if anyone else starts using it it might not be. I'd > prefer to give this a very specific name like "ShouldExposeGetUserData" or > something. Renamed.
Attachment #717066 -
Attachment is obsolete: true
Attachment #717194 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Comment 27•11 years ago
|
||
Attachment #717067 -
Attachment is obsolete: true
Attachment #717067 -
Flags: review?(jonas)
Attachment #717195 -
Flags: review?(jonas)
Comment 28•11 years ago
|
||
Comment on attachment 717194 [details] [diff] [review] Part 1: Implement nsINode::ShouldExposeUserData Review of attachment 717194 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/public/nsINode.h @@ +1134,5 @@ > > return NS_OK; > } > > + static bool ShouldExposeUserData(JSContext* aCx, JSObject* /* unused */); Please add some comments here stating that this refers to the getUserData/setUserData accessors. When I read this, I it sounds like a privacy issue. ;-)
Attachment #717194 -
Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 717195 [details] [diff] [review] Part 2: Hide getUserData and setUserData from content I won't have time to look at this. Looks like bz has been involved here.
Attachment #717195 -
Flags: review?(jonas) → review?(bzbarsky)
![]() |
||
Comment 30•11 years ago
|
||
Comment on attachment 717195 [details] [diff] [review] Part 2: Hide getUserData and setUserData from content r=me
Attachment #717195 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 31•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2045d4808bf https://hg.mozilla.org/integration/mozilla-inbound/rev/6b597b719265
Flags: in-testsuite+
Comment 32•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b2045d4808bf https://hg.mozilla.org/mozilla-central/rev/6b597b719265
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment 33•11 years ago
|
||
I've added this bug to the compatibility doc. Please correct the info if wrong. https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22 Also, please update the following docs: https://developer.mozilla.org/en-US/docs/DOM/Node.getUserData https://developer.mozilla.org/en-US/docs/DOM/Node.setUserData
Comment 34•11 years ago
|
||
> https://developer.mozilla.org/en-US/docs/DOM/Node.getUserData
> https://developer.mozilla.org/en-US/docs/DOM/Node.setUserData
Updated.
Keywords: dev-doc-needed → dev-doc-complete
Updated•11 years ago
|
Keywords: site-compat
You need to log in
before you can comment on or make changes to this bug.
Description
•