1088228 - Stop putting random objects on function scope chains in Gecko

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Bug 1083648 fixes the inline event handler case so we only have With objects and a global.

The remaining cases are the ones that use JS_CloneFunctionObject: XBL methods, XBL properties, XBL event handlers.

I'm going to fix this the same way: Add a JS::CloneFunctionObject that takes a scope chain vector and wraps it up appropriately.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Introduce an nsINode API for getting the scope chain parent for a given node

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Create an nsJSUtils API for building the scope chain for a given Element

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Use the new nsJSUtils API for event handlers

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Add a version of JS_CloneFunctionObject that allows passing in a scope chain

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Use the new CloneFunctionObject in XBL

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Simplify the DOM GetParentObject setup, now that we no longer rely on it for scope chains

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Use the new nsJSUtils API for event handlers

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Use the new CloneFunctionObject in XBL

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8511444 [details] [diff] [review]
part 4.  Add a version of JS_CloneFunctionObject that allows passing in a scope chain

Review of attachment 8511444 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ +3922,5 @@
>  }
>  
> +namespace JS {
> +
> +extern JS_PUBLIC_API(JSObject *)

No extern here.

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 8511441 [details] [diff] [review]
part 1.  Introduce an nsINode API for getting the scope chain parent for a given node

Review of attachment 8511441 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/nsINode.h
@@ +395,5 @@
>  
>  public:
>    mozilla::dom::ParentObject GetParentObject() const; // Implemented in nsIDocument.h
>  
> +  virtual nsINode* GetScopeChainParent() const;

Please document, in particular what it means if this returns null.

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 8511442 [details] [diff] [review]
part 2.  Create an nsJSUtils API for building the scope chain for a given Element

Review of attachment 8511442 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsJSUtils.h
@@ +119,5 @@
>                                   JS::Handle<JSObject*> aScopeObject,
>                                   JS::CompileOptions &aCompileOptions,
>                                   void **aOffThreadToken = nullptr);
>  
> +  // Returns false if an exception got thrown on aCx.

Actually, could you document that this can take null for aElement?

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 8511512 [details] [diff] [review]
part 3.  Use the new nsJSUtils API for event handlers

Review of attachment 8511512 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Please document, in particular what it means if this returns null.

  /**
   * Return the scope chain parent for this node, for use in things
   * like event handler compilation.  Returning null means to use the
   * global object as the scope chain parent.
   */

> Actually, could you document that this can take null for aElement?

Done.

   https://hg.mozilla.org/integration/mozilla-inbound/rev/55f4818378e4
   https://hg.mozilla.org/integration/mozilla-inbound/rev/920d50e84a17
   https://hg.mozilla.org/integration/mozilla-inbound/rev/49ac8f33ab70
   https://hg.mozilla.org/integration/mozilla-inbound/rev/2d449a2b4e1c
   https://hg.mozilla.org/integration/mozilla-inbound/rev/de692c3335f2

Comment 14

[Boris Zbarsky [:bzbarsky]]

Er, oops, and https://hg.mozilla.org/integration/mozilla-inbound/rev/13ecff800114

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a37fedd51b7d for static analysis bustage:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-st-an-debug/1414705466/mozilla-inbound-linux64-st-an-debug-bm74-build1-build707.txt.gz


../../dom/xbl/nsXBLProtoImplMethod.o: In function `nsXBLProtoImplAnonymousMethod::Execute(nsIContent*, JSAddonId*)':
/builds/slave/m-in-l64-st-an-d-0000000000000/build/dom/xbl/nsXBLProtoImplMethod.cpp:304: undefined reference to `nsINode::AsElement()'
/usr/bin/ld: libxul.so: hidden symbol `_ZN7nsINode9AsElementEv' isn't defined
/usr/bin/ld: final link failed: Nonrepresentable section on output
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[5]: *** [libxul.so] Error 1
make[4]: *** [toolkit/library/target] Error 2
make[4]: *** Waiting for unfinished jobs....
testPreserveJitCode.o

Comment 16

[Boris Zbarsky [:bzbarsky]]

Included Element.h in that file and pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6742368cd098
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c884f8186efc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6f444107021e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cb7fe25656fc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f5e241cd0b82
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/411d5252365b

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6742368cd098
https://hg.mozilla.org/mozilla-central/rev/c884f8186efc
https://hg.mozilla.org/mozilla-central/rev/6f444107021e
https://hg.mozilla.org/mozilla-central/rev/cb7fe25656fc
https://hg.mozilla.org/mozilla-central/rev/f5e241cd0b82
https://hg.mozilla.org/mozilla-central/rev/411d5252365b