842372 - Make getUserData and setUserData ChromeOnly

Status: RESOLVED FIXED

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

Description

[Masatoshi Kimura [:emk]]

Attachment: patch

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.

Comment 1

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=5ef9fdc9d43f

Comment 2

[Boris Zbarsky [:bzbarsky]]

Doesn't this break addons that use get/setUserData from XBL bindings, if any?

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: mochitest

(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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Masatoshi Kimura [:emk]]

(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

[Boris Zbarsky [:bzbarsky]]

Hmm.  Yes, that's teh one I was thinking of.

Bobby, any idea why this doesn't work with the pref flipped?

Comment 7

[Masatoshi Kimura [:emk]]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Masatoshi Kimura [:emk]]

Do XBL compartments have the system principal?
https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/AccessCheck.cpp?rev=1c2e7ae47afc#90

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

No.

Comment 11

[Masatoshi Kimura [:emk]]

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

[Boris Zbarsky [:bzbarsky]]

> 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

[Bobby Holley (:bholley)]

(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

[Boris Zbarsky [:bzbarsky]]

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 15

[Masatoshi Kimura [:emk]]

Will [Func="IsCallerChromeOrXBL"] work once bug 838691 has been landed?

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: Hide getUserData and setUserData from content

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 716269 [details] [diff] [review]
Hide getUserData and setUserData from content

Very nice!

Comment 19

[Bobby Holley (:bholley)]

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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

[Bobby Holley (:bholley)]

(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.

Comment 22

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Implement nsINode::IsChromeOrXBL

Comment 23

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Hide getUserData and setUserData from content

Comment 24

[Bobby Holley (:bholley)]

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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 26

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Implement nsINode::ShouldExposeUserData

(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.

Comment 27

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Hide getUserData and setUserData from content

Comment 28

[Bobby Holley (:bholley)]

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. ;-)

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 717195 [details] [diff] [review]
Part 2: Hide getUserData and setUserData from content

r=me

Comment 31

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2045d4808bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b597b719265

Comment 32

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/b2045d4808bf
https://hg.mozilla.org/mozilla-central/rev/6b597b719265

Comment 33

[Kohei Yoshino [:kohei]]

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

[Kohei Yoshino [:kohei]]

> https://developer.mozilla.org/en-US/docs/DOM/Node.getUserData
> https://developer.mozilla.org/en-US/docs/DOM/Node.setUserData

Updated.