Last Comment Bug 758912 - Don't use JSCLASS_NEW_RESOLVE_GETS_START in XBL code
: Don't use JSCLASS_NEW_RESOLVE_GETS_START in XBL code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XBL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 761439 761452 761462 768750
Blocks: 758913
  Show dependency treegraph
 
Reported: 2012-05-26 14:56 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-10-18 22:42 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (14.57 KB, patch)
2012-06-05 17:19 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bzbarsky: review-
Details | Diff | Splinter Review
Interdiff of comment-addressal (9.40 KB, patch)
2012-06-11 17:56 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Oops, forgot to qref (10.39 KB, patch)
2012-06-11 18:02 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Original patch, unbitrotted (14.42 KB, patch)
2012-06-19 23:38 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Unbitrotted interdiff against unbitrotted patch (10.44 KB, patch)
2012-06-19 23:40 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bzbarsky: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-26 14:56:12 PDT
JSCLASS_NEW_RESOLVE_GETS_START has semantics that are incompatible with those in the ECMAScript property access algorithms, and I'd like to remove it from the JSAPI.  In order to do that, I need to remove the one use of it in XBL, where the resolve hook on an XBL prototype object resolves <field/> properties as properties on the original ("start") XBL element.

My plan is to have the resolve hook instead resolve a getter/setter pair onto the XBL prototype object.  The getter/setter, when called, *will* be passed the start object, and thus each can define the field on the start object when used.  It'll be a bit tricky, to be sure, but it should work.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-05 17:19:02 PDT
Created attachment 630378 [details] [diff] [review]
Patch

This is a little bit messy how it's all done, to be sure.  But it's definitely worth it to simplify every API that performs property accesses and to make them more like the spec algorithms.

Oh, this passed try:

https://tbpl.mozilla.org/?tree=Try&rev=1b4a0a4cdfb2
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-06-08 21:34:54 PDT
Comment on attachment 630378 [details] [diff] [review]
Patch

Sorry for the lag here; it took a while to find an uninterrupted chunk of time when I could really immerse myself in this.

>+static const uint32_t XBLPROTO_SLOT = 0;
>+static const uint32_t FIELD_SLOT = 1;

Please document what will be stored in these slots, and on which sort of objects.

>+InstallXBLField(JSContext* cx,
>+                JS::Handle<JSObject*> callee, JS::Handle<JSObject*> xblNode,

Maybe s/xblNode/thisObj/ ?

>-  if (~nodeClass->flags &
>-      (JSCLASS_HAS_PRIVATE | JSCLASS_PRIVATE_IS_NSISUPPORTS)) {
>-    nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_UNEXPECTED);

Hmm.  Why did we move this check so far away from where this actually matters?

At the very least, we should be asserting this here, and maybe documenting before the method, because otherwise this code sure looks exploitable.

>+  // First ensure |this| is a reasonable XBL node.

"XBL bound node"

>-    // Looks like whatever |origObj| is it's not our nsIContent.  It might well
>-    // be the proto our binding installed, however, where the private is the
>-    // nsXBLDocumentInfo, so just baul out quietly.  Do NOT throw an exception
>-    // here.
>-    // We could make this stricter by checking the class maybe, but whatever
>-    return JS_TRUE;
>+    // Bail quietly if this wasn't a wrapped native at all -- we only error if
>+    // it was but we can't get a node from it.

Which raises the obvious question of "why?".  The old comment explained exactly the common case we hit where we didn't want to throw an exception... Please restore the rationale for this behavior?

>+  // Because of the possibility (due to XBL binding inheritance) that |this|
>+  // might be in a different compartment from the callee

Hmm.  That would be pretty odd, I would think....  I guess entering the callee compartment can't hurt, but I'm not sure I see why it's needed.

>+  // compartment to access its reserved slots.  (INCEPTION *dong*)

Can you nix the parenthetical, please?  ;)

>+ValueIsXBLNode(JS::Handle<JS::Value> v)

All this is really testint for is ValueHasISupportsPrivate(), so I suggest calling it that.

Though see more below on value vs object.

>+FieldAccessorGuard(JSContext *cx, unsigned argc, JS::Value *vp, JSNative native, JSObject **thisObj)

It'd be good to document what this is trying to do.  For one thing then I could tell whether it's managing it.  ;)

>+  JS::Rooted<JS::Value> thisv(cx, JS_THIS_VALUE(cx, vp));

Is there a reason to not do:

  JS::Rooted<JSObject*> thisObj(cx, JS_THIS(cx, vp));
  if (!thisObj) {

etc?  Would avoid having to do things like thisv.reference().isObject() and v.value().toObject() (which would get replaces with a null-check and "obj" respectively).

>+  return JS_CallNonGenericMethodOnProxy(cx, argc, vp, native, protoClass);

So this is trying to handle the case when |this| is actually a security wrapper around a node?  And will throw if it's some other object?

I can't understand from the documentation from this method why protoClass is the right JS::Class* to pass in here.  Why is it?  Please document...

>+FieldGetter(JSContext *cx, unsigned argc, JS::Value *vp)
>+  if (!thisObj) {
>+    return true;

Document that this means FieldAccessorGuard did all the work?

>+  JS::Rooted<JS::Value> v(cx);
>+  if (!JS_GetPropertyById(cx, thisObj, id, v.address())) {
>+    return false;
>+  }
>+  JS_SET_RVAL(cx, vp, v);
>+  return true;

Why can that not just be:

  return JS_GetPropertyById(cx, thisObj, id, vp);

?  If there's a good reason, it's worth documenting.

>+XBLResolve(JSContext *cx, JSHandleObject obj, JSHandleId id, unsigned flags,
>+  if (!field || field->IsEmpty()) {

There didn't use to be an IsEmpty() check here, afaict.  Why add it?  Just because in the empty case there is no point invoking all this machinery for a getter or setter?

The last looks fine, I think.

I'd like to see an interdiff addressing the above nits, if possible.

This is pretty awesome.  I like the element enumerate hook dying.  ;)
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-11 17:56:21 PDT
Created attachment 632093 [details] [diff] [review]
Interdiff of comment-addressal

(In reply to Boris Zbarsky (:bz) from comment #2)
> Please document what will be stored in these slots, and on which sort of
> objects.

I put an overview doc comment which also addresses this, I hope.

> Maybe s/xblNode/thisObj/ ?

Sure.

> >-  if (~nodeClass->flags &
> >-      (JSCLASS_HAS_PRIVATE | JSCLASS_PRIVATE_IS_NSISUPPORTS)) {
> >-    nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_UNEXPECTED);
> 
> Hmm.  Why did we move this check so far away from where this actually
> matters?

It is where it actually matters, at least if you want the object-is-wrapped-or-proxied case to be dealt with as simply and summarily as possible.  The big problem is that knowing you have an nsISupportsy object isn't enough to say that the method will install anything for the field, because of that nsXBLDocumentInfo case I don't understand at all.  If that were a throwing case, we could put more of the tests here.  I don't see a way to refactor this logic and keep the this-testing conditions closer together if that silently-do-nothing-with-weird-this case must be preserved.

> At the very least, we should be asserting this here, and maybe documenting
> before the method, because otherwise this code sure looks exploitable.

Added the assert, and in a way which meshes nicely with surrounding comments, if I do say so myself.

> >-    // Looks like whatever |origObj| is it's not our nsIContent.  It might well
> >-    // be the proto our binding installed, however, where the private is the
> >-    // nsXBLDocumentInfo, so just baul out quietly.  Do NOT throw an exception
> >-    // here.
> >-    // We could make this stricter by checking the class maybe, but whatever
> >-    return JS_TRUE;
> >+    // Bail quietly if this wasn't a wrapped native at all -- we only error if
> >+    // it was but we can't get a node from it.
> 
> Which raises the obvious question of "why?".  The old comment explained
> exactly the common case we hit where we didn't want to throw an exception...
> Please restore the rationale for this behavior?

I restored the original comment.  Which to be clear, I don't understand a word of beyond the first sentence, and which was why I removed it.  Plus since you can now get arbitrary nsISupportsy objects here that aren't wrapped natives, it seems overly specific.  That's just one case of the concern, no different from any of a bunch of others script might cons up if it wanted.

> >+  // Because of the possibility (due to XBL binding inheritance) that |this|
> >+  // might be in a different compartment from the callee
> 
> Hmm.  That would be pretty odd, I would think....  I guess entering the
> callee compartment can't hurt, but I'm not sure I see why it's needed.

It's not at all odd with inheritance.  I hit the case in crashtest, or mochitest, or somewhere in our test suites.  Since bindings are each compiled in their own globals, I think any binding inheritance where a field on an inherited binding is accessed will trigger this.  I want to say the case in question I was hitting was for a "this._userCleared" access (_userCleared being a field) inside the video controls (inheriting from scale binding for volume controls or maybe seeking), but I'm not 100% sure of that recollection.

> >+  // compartment to access its reserved slots.  (INCEPTION *dong*)
> 
> Can you nix the parenthetical, please?  ;)

Boo-urns, you're no fun.  :-P

> >+ValueIsXBLNode(JS::Handle<JS::Value> v)
> 
> All this is really testint for is ValueHasISupportsPrivate(), so I suggest
> calling it that.

Fair enough.

> >+FieldAccessorGuard(JSContext *cx, unsigned argc, JS::Value *vp, JSNative native, JSObject **thisObj)
> 
> It'd be good to document what this is trying to do.  For one thing then I
> could tell whether it's managing it.  ;)

Basically it's implementing the rest of the JS_CallNonGenericMethodOnProxy idiom, documented with an example in jsapi.h.  I added a little commentary on this, but I really want to avoid belaboring what jsapi.h should explain.

Which isn't to say I like the idiom; really it's pretty awful.  But nobody around here has any better ideas, and it's not so bad once you get used to it.

> >+  JS::Rooted<JS::Value> thisv(cx, JS_THIS_VALUE(cx, vp));
> 
> Is there a reason to not do:
> 
>   JS::Rooted<JSObject*> thisObj(cx, JS_THIS(cx, vp));
>   if (!thisObj) {

Not too much, except that I don't much like the implicit boxing up of |this| here.  I guess it doesn't matter much here, for this garbage; switched.

> >+  return JS_CallNonGenericMethodOnProxy(cx, argc, vp, native, protoClass);
> 
> So this is trying to handle the case when |this| is actually a security
> wrapper around a node?  And will throw if it's some other object?

Yes.

> I can't understand from the documentation from this method why protoClass is
> the right JS::Class* to pass in here.  Why is it?  Please document...

I added a comment on this.  Really it's only passed to help the method report a more informative error, saying the expected type of object that should have been passed.  But really that's wrong, because any of a number of classes of objects could be passed.  It's not an exact class-check, so the idea of saying one class was expected is just dumb.  I think the "expected instance of <classname>" part of the error message should be removed, myself.  But that's a separate bug.  Passing protoClass here gets vaguely reasonable error messages now, which is enough for this bug's purposes, I think.

> >+FieldGetter(JSContext *cx, unsigned argc, JS::Value *vp)
> >+  if (!thisObj) {
> >+    return true;
> 
> Document that this means FieldAccessorGuard did all the work?

Added a little comment, but really it should be something you pick up when you learn the guarding idiom here.  (Which I repeat, again, is the worst thing in the world, except for all the other alternatives.)

> >+  JS::Rooted<JS::Value> v(cx);
> >+  if (!JS_GetPropertyById(cx, thisObj, id, v.address())) {
> >+    return false;
> >+  }
> >+  JS_SET_RVAL(cx, vp, v);
> >+  return true;
> 
> Why can that not just be:
> 
>   return JS_GetPropertyById(cx, thisObj, id, vp);
> 
> ?  If there's a good reason, it's worth documenting.

|vp| is only the return value location for legacy reasons now.  JS_SET_RVAL is the correct way to set a return value now.  As jsapi.h says, "These are not (yet) mandatory macros, but new code outside of the engine should use them."  Hence this verbose dance.

> >+XBLResolve(JSContext *cx, JSHandleObject obj, JSHandleId id, unsigned flags,
> >+  if (!field || field->IsEmpty()) {
> 
> There didn't use to be an IsEmpty() check here, afaict.  Why add it?  Just
> because in the empty case there is no point invoking all this machinery for
> a getter or setter?

There was one in nsXBLProtoImplField::InstallField.  (I made it use the IsEmpty test method in this interdiff to point out the place to you.)  Just because you call the install-field method doesn't mean the field was installed.  And if the field wouldn't be installed, it's as if the resolve hook didn't resolve it.  And to emulate that behavior, the resolve hook has to look at the field guts to make sure not to resolve the field accessor if the field's empty.

Yeah, dumb.  There was a test that sniffed out this specific case.  Yay.  And UGH WHOEVER THOUGHT THESE SEMANTICS WERE A GOOD IDEA.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-11 18:02:49 PDT
Created attachment 632094 [details] [diff] [review]
Oops, forgot to qref

Oh, and the delay seemed pretty reasonable, or at least it was except that I have the patience of a two-year-old when it comes to wanting to stop having to hack XBL code that implicates tests in a bunch of test suites in random, not-well-contained places that makes try easily the best way to test for failures.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-19 23:38:15 PDT
Created attachment 634774 [details] [diff] [review]
Original patch, unbitrotted
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-19 23:40:42 PDT
Created attachment 634775 [details] [diff] [review]
Unbitrotted interdiff against unbitrotted patch

This applies atop the unbitrotted patch, which itself applies to current m-i.  Not clearing/obsoleting the previous interdiff just in case you have Splinter review comments pending on it, as last I recall doing so deep-sixes those comments... :-\
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-06-21 21:02:29 PDT
Comment on attachment 634775 [details] [diff] [review]
Unbitrotted interdiff against unbitrotted patch

r=me.  Thank you for the comments!
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-22 02:04:27 PDT
(In reply to Boris Zbarsky (:bz) from comment #7)
> Thank you for the comments!

Except for the (INCEPTION) one.  :-P

https://hg.mozilla.org/integration/mozilla-inbound/rev/86a43c804932
Comment 9 Ed Morley [:emorley] 2012-06-22 09:16:44 PDT
https://hg.mozilla.org/mozilla-central/rev/86a43c804932

Note You need to log in before you can comment on or make changes to this bug.