Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Make getUserData and setUserData ChromeOnly

RESOLVED FIXED in mozilla22

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-complete, site-compat})

unspecified
mozilla22
addon-compat, dev-doc-complete, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 715218 [details] [diff] [review]
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.
Attachment #715218 - Flags: review?(jonas)
(Assignee)

Updated

5 years ago
Keywords: addon-compat, dev-doc-needed
(Assignee)

Comment 1

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

Comment 2

5 years ago
Doesn't this break addons that use get/setUserData from XBL bindings, if any?
(Assignee)

Comment 3

5 years ago
Created attachment 715661 [details] [diff] [review]
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

5 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

5 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

5 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

5 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

5 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

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

Comment 11

5 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

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

5 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

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

Comment 16

5 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)

Updated

5 years ago
Depends on: 838691
(Assignee)

Comment 17

5 years ago
Created attachment 716269 [details] [diff] [review]
Hide getUserData and setUserData from content
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

5 years ago
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

Comment 20

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

5 years ago
Created attachment 717066 [details] [diff] [review]
Part 1: Implement nsINode::IsChromeOrXBL
Attachment #716269 - Attachment is obsolete: true
Attachment #716269 - Flags: review?(jonas)
Attachment #717066 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 23

5 years ago
Created attachment 717067 [details] [diff] [review]
Part 2: Hide getUserData and setUserData from content
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-

Comment 25

5 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

5 years ago
Created attachment 717194 [details] [diff] [review]
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.
Attachment #717066 - Attachment is obsolete: true
Attachment #717194 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 27

5 years ago
Created attachment 717195 [details] [diff] [review]
Part 2: Hide getUserData and setUserData from content
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 30

5 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

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2045d4808bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b597b719265
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/b2045d4808bf
https://hg.mozilla.org/mozilla-central/rev/6b597b719265
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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
> 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

4 years ago
Keywords: site-compat

Updated

4 years ago
Depends on: 876228
You need to log in before you can comment on or make changes to this bug.