Provide a host defined field that can be set on any global script or module

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
10 months ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Blocks 1 bug)

61 Branch
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed, firefox64 fixed)

Details

(Whiteboard: [overhead:12k])

Attachments

(1 attachment, 3 obsolete attachments)

To implement dynamic module imports we'll need a way for the script loader to associate data with top-level (global) scripts as well as modules.

(In the spec this is Script Record's [[HostDefined]] field.)
This removes ModuleObject's HostDefined slot and adds a void* pointer to top level scopes, adding a base class for global and module scopes.  I did it like this to avoid adding an extra field to every JSScript since most of these are not top-level.

After I did this I realised that it might make more sense to put this in the script's non-shared data.  I had a go at doing this but it turned out to be a pain (alignment is important, you don't always know whether the script requires a private when this is set up, you have to calculate a pointer to it...). What do you think, is this OK or shall I have a another go?
Attachment #8998906 - Flags: review?(jdemooij)
Update the script loader to use the new API for storing a private pointer on a JSScript.
Attachment #8998910 - Flags: review?(hsivonen)
Comment on attachment 8998906 [details] [diff] [review]
bug1482153-host-defined-field-1

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

I think this is fine, it's just annoying that JSScript::isTopLevel() also returns true for eval scripts. However I think in that case we're guaranteed to MOZ_CRASH.
Attachment #8998906 - Flags: review?(jdemooij) → review+
Attachment #8998910 - Flags: review?(hsivonen) → review+
(In reply to Jan de Mooij [:jandem] from comment #3)
> I think this is fine, it's just annoying that JSScript::isTopLevel() also
> returns true for eval scripts. However I think in that case we're guaranteed
> to MOZ_CRASH.

Ok thanks, I can add some comments to make this clearer.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4129380f8318
Replace ModuleObject's host defined field with one on top-level JSScripts r=jandem r=hsivonen
Backed out for assertion failures at ModuleScript.cpp:59.

backout:  https://hg.mozilla.org/integration/mozilla-inbound/rev/09c51359bb20f1ebf9adfcc893f3199c7389d101

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8289b70dc0dbbe144f0081c7d99add92707efe79

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=193674065&repo=mozilla-inbound&lineNumber=35756

[task 2018-08-13T16:36:20.056Z] 16:36:20     INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=test262/language/module-code/namespace/internals/set-prototype-of.js;module
[task 2018-08-13T16:36:20.057Z] 16:36:20     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=test262/language/module-code/namespace/internals/set-prototype-of.js;module | 3870 / 10837 (35%)
[task 2018-08-13T16:36:20.092Z] 16:36:20     INFO - Assertion failure: JS::GetTopLevelScriptPrivate(mScript) == this, at /builds/worker/workspace/build/src/dom/script/ModuleScript.cpp:59
[task 2018-08-13T16:36:42.772Z] 16:36:42     INFO - #01: mozilla::dom::ModuleScript::cycleCollection::Unlink(void*) [dom/script/ModuleScript.cpp:25]
[task 2018-08-13T16:36:42.781Z] 16:36:42     INFO - 
[task 2018-08-13T16:36:42.782Z] 16:36:42     INFO - #02: nsCycleCollector::CollectWhite() [xpcom/base/nsCycleCollector.cpp:3467]
[task 2018-08-13T16:36:42.784Z] 16:36:42     INFO - 
[task 2018-08-13T16:36:42.785Z] 16:36:42     INFO - #03: nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) [xpcom/base/nsCycleCollector.cpp:3836]
[task 2018-08-13T16:36:42.786Z] 16:36:42     INFO - 
[task 2018-08-13T16:36:42.787Z] 16:36:42     INFO - #04: nsCycleCollector_collectSlice(js::SliceBudget&, bool) [xpcom/base/nsCycleCollector.cpp:4420]
[task 2018-08-13T16:36:42.787Z] 16:36:42     INFO - 
[task 2018-08-13T16:36:42.787Z] 16:36:42     INFO - #05: nsJSContext::RunCycleCollectorSlice(mozilla::TimeStamp) [dom/base/nsJSEnvironment.cpp:1568]
[task 2018-08-13T16:36:42.788Z] 16:36:42     INFO - 
[task 2018-08-13T16:36:42.789Z] 16:36:42     INFO - #06: ICCRunnerFired [dom/base/nsJSEnvironment.cpp:1623]
[task 2018-08-13T16:36:42.789Z] 16:36:42     INFO - 
[task 2018-08-13T16:36:42.790Z] 16:36:42     INFO - #07: mozilla::IdleTaskRunner::Run() [gcc/include/c++/6.4.0/functional:2127]
[task 2018-08-13T16:36:42.790Z] 16:36:42     INFO - 
[task 2018-08-13T16:36:42.790Z] 16:36:42     INFO - #08: nsJSContext::RunNextCollectorTimer(JS::gcreason::Reason, mozilla::TimeStamp) [dom/base/nsJSEnvironment.cpp:2042]
[task 2018-08-13T16:36:42.790Z] 16:36:42     INFO - 
[task 2018-08-13T16:36:42.791Z] 16:36:42     INFO - #09: nsDOMWindowUtils::RunNextCollectorTimer() [dom/base/nsDOMWindowUtils.cpp:1264]
[task 2018-08-13T16:36:42.792Z] 16:36:42     INFO - 
[task 2018-08-13T16:36:42.793Z] 16:36:42     INFO - #10: NS_InvokeByIndex
[task 2018-08-13T16:36:42.793Z] 16:36:42     INFO - 
[task 2018-08-13T16:36:42.794Z] 16:36:42     INFO - #11: CallMethodHelper::Call() [js/xpconnect/src/xpcprivate.h:712]
[task 2018-08-13T16:36:42.794Z] 16:36:42     INFO - 
[task 2018-08-13T16:36:42.795Z] 16:36:42     INFO - #12: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [js/public/RootingAPI.h:1011]
[task 2018-08-13T16:36:42.796Z] 16:36:42     INFO - 
[task 2018-08-13T16:36:42.797Z] 16:36:42     INFO - #13: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) [js/xpconnect/src/XPCWrappedNativeJSOps.cpp:893]
[task 2018-08-13T16:36:42.797Z] 16:36:42     INFO - 
[task 2018-08-13T16:36:42.798Z] 16:36:42     INFO - #14: ??? (???:???)
[task 2018-08-13T16:36:42.799Z] 16:36:42     INFO - [Parent 977, Gecko_IOThread] WARNING: pipe error (131): Connection reset by peer: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 356
[task 2018-08-13T16:36:42.800Z] 16:36:42     INFO - ###!!! [Parent][MessageChannel] Error: (msgtype=0x17007C,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
[task 2018-08-13T16:36:42.800Z] 16:36:42     INFO - A content process crashed and MOZ_CRASHREPORTER_SHUTDOWN is set, shutting down
[task 2018-08-13T16:36:42.801Z] 16:36:42     INFO - JavaScript error: chrome://reftest/content/reftest.jsm, line 1558: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIPropertyBag2.getPropertyAsAString]
Flags: needinfo?(jcoppeard)
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da82ea6820ee
Replace ModuleObject's host defined field with one on top-level JSScripts r=jandem r=hsivonen
https://hg.mozilla.org/mozilla-central/rev/da82ea6820ee
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(jcoppeard)
As explained in bug 1481196 comment 6 I'm going to back this out.

Just running this past you for cursory review as it's not a straight backout due to intervening changes.
Attachment #9014065 - Flags: review?(jdemooij)
Comment on attachment 9014065 [details] [diff] [review]
backout-bug1482153-host-defined-field

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

Good find.
Attachment #9014065 - Flags: review?(jdemooij) → review+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Priority: -- → P3
Blocks: 1499140
Here's a new approach that uses a new reserved slot to ScriptSourceObject and uses that to store the host-defined field.  This uses a JS::Value so that we can use a JSObject for the shell and a pointer inside a PrivateValue for the browser.

Using ScriptSourceObject is really helpful for dynamic import because it makes it easy to find this private given a call to import() that may be an a deeply nested scope.

The patch also changes the module hooks to pass the private value rather than the module object.  We don't have an equivalent object for scripts to pass and it makes more sense not to expose this.

I changed the terminology to 'private' from 'host defined field' because the latter makes for unwieldy function names and we already use 'private' for the in our other APIs.

This does push up the size of ScriptSource object but I think it might be possible to move some of the existing data on this (DOM element and attribute name) to the browser's private data in the future.

I'll request separate review for the browser changes.
Attachment #8998906 - Attachment is obsolete: true
Attachment #8998910 - Attachment is obsolete: true
Attachment #9014065 - Attachment is obsolete: true
Attachment #9017255 - Flags: review?(jdemooij)
Comment on attachment 9017255 [details] [diff] [review]
bug1482153-script-or-module-private

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

Nice.
Attachment #9017255 - Flags: review?(jdemooij) → review+
Comment on attachment 9017255 [details] [diff] [review]
bug1482153-script-or-module-private

Requesting additional review for browser changes.  This is changing the module hooks to pass the private JS::Value directly rather than passing a ModuleObject and renaming the APIs for getting/setting the private value.
Attachment #9017255 - Flags: review?(hsivonen)
Comment on attachment 9017255 [details] [diff] [review]
bug1482153-script-or-module-private

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

Really an rs+ rather than an r+.
Attachment #9017255 - Flags: review?(hsivonen) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/180eb0ea89bc
Provide a way of associating a private value with a script or module r=jandem rs=hsivonen
https://hg.mozilla.org/mozilla-central/rev/180eb0ea89bc
Status: REOPENED → RESOLVED
Closed: Last year10 months ago
Resolution: --- → FIXED
This appears to have regressed JS memory usage in the base AWSY test by 12k. Jon, is that expected? Is there anything we can do to avoid that?
Flags: needinfo?(jcoppeard)
Whiteboard: [overhead:12k]
(In reply to Eric Rahm [:erahm] from comment #18)
It was expected that it might increase memory usage a little.  How significant is 12K?  I'll file a bug for the possible mitigation mentioned in comment 12, but this is something that would have to be done after bug 1342012.
Flags: needinfo?(jcoppeard)
(In reply to Jon Coppeard (:jonco) from comment #19)
> (In reply to Eric Rahm [:erahm] from comment #18)
> It was expected that it might increase memory usage a little.  How
> significant is 12K?  I'll file a bug for the possible mitigation mentioned
> in comment 12, but this is something that would have to be done after bug
> 1342012.

We consider 12K somewhat significant for the JS base measurement (essentially JS memory usage for a content process with about:blank loaded), thanks for filing the follow up.
You need to log in before you can comment on or make changes to this bug.