Stop putting random objects on function scope chains in Gecko

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

({addon-compat})

unspecified
mozilla36
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

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.
Depends on: 1083648
Blocks: 1089026
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8511443 - Attachment is obsolete: true
Attachment #8511443 - Flags: review?(peterv)
Attachment #8511445 - Attachment is obsolete: true
Attachment #8511445 - Flags: review?(peterv)

Comment 9

5 years ago
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.
Attachment #8511444 - Flags: review?(shu) → review+
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.
Attachment #8511441 - Flags: review?(peterv) → review+
Attachment #8511442 - Flags: review?(peterv) → review+
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 on attachment 8511512 [details] [diff] [review]
part 3.  Use the new nsJSUtils API for event handlers

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

Nice.
Attachment #8511512 - Flags: review?(peterv) → review+
Attachment #8511513 - Flags: review?(peterv) → review+
Attachment #8511446 - Flags: review?(peterv) → review+
> 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
Target Milestone: --- → mozilla36
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
Flags: needinfo?(bzbarsky)
Depends on: 1092171
Keywords: addon-compat
Depends on: 1095660
You need to log in before you can comment on or make changes to this bug.