Closed Bug 842372 Opened 11 years ago Closed 11 years ago

Make getUserData and setUserData ChromeOnly

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

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)

Attached patch patch (obsolete) — 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)
Doesn't this break addons that use get/setUserData from XBL bindings, if any?
Attached patch mochitest (obsolete) — Splinter Review
(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]?
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.
(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.
Hmm.  Yes, that's teh one I was thinking of.

Bobby, any idea why this doesn't work with the pref flipped?
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.
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.
> 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.
(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.
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
Will [Func="IsCallerChromeOrXBL"] work once bug 838691 has been landed?
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.
Depends on: 838691
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 on attachment 716269 [details] [diff] [review]
Hide getUserData and setUserData from content

Very nice!
Attachment #716269 - Flags: review?(bzbarsky) → review+
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
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.
(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.
Attachment #716269 - Attachment is obsolete: true
Attachment #716269 - Flags: review?(jonas)
Attachment #717066 - Flags: review?(bobbyholley+bmo)
Attachment #717067 - Flags: review?(jonas)
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-
> 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.
(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)
Attachment #717067 - Attachment is obsolete: true
Attachment #717067 - Flags: review?(jonas)
Attachment #717195 - Flags: review?(jonas)
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 on attachment 717195 [details] [diff] [review]
Part 2: Hide getUserData and setUserData from content

r=me
Attachment #717195 - Flags: review?(bzbarsky) → review+
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
Keywords: site-compat
Depends on: 876228
You need to log in before you can comment on or make changes to this bug.