1512509 - Clone ScriptSourceObject when cloning scripts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Right now script->sourceObject can be a CCW for a ScriptSourceObject when we clone a script across compartments. This introduces a lot of complexity and bugs (like bug 1406437).

I think it would be simpler to clone the ScriptSourceObject when we clone scripts cross-compartment. The element/introductionScript/etc stored in ScriptSourceObject could be moved into a separate object and ScriptSourceObject could store this in a slot (this shared object could be a CCW but there aren't that many places that access it so it's still simpler than having CCW script->sourceObject).

Comment 1

[Jan de Mooij [:jandem]]

One question is what to do with the "private" slot in ScriptSourceObject - should it be preserved across JSScript/ScriptSourceObject clones?

Comment 2

[Jon Coppeard (:jonco)]

This slot contains an AddRef()ed nsISupports pointer in a private value so it's not safe to make a copy of it.  I'm not sure whether this slot will be set in any situation where we clone the SSO.  Probably the best thing to do is not copy this slot and if that causes problems we'll revisit this.

Comment 3

[Jan de Mooij [:jandem]]

I wrote a patch for this yesterday:

https://hg.mozilla.org/try/rev/099163b3a8fe9e2dbb58cebb12c26efe6307fc4c

It works and is green. I like that JSScript now has a GCPtr<ScriptSourceObject*> instead of a GCPtrObject and getting the SSO or ScriptSource no longer requires any unwrapping.

However this ScriptSourceSharedObject comes with some complexity because it's a new thing in our script hierarchy. I now think it might be simpler to give ScriptSourceObject a "canonical ScriptSourceObject" slot: we would use this canonical SSO for the element/private/introductionScript meta data and when clone a SSO we would just propagate the canonical SSO.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1512509 - Clone ScriptSourceObject when cloning scripts. r?tcampbell

This fixes bug 1406437. It also simplifies JSScript because it now always stores
a ScriptSourceObject directly instead of a CCW for one.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> I now think it might be simpler to
> give ScriptSourceObject a "canonical ScriptSourceObject" slot: we would use
> this canonical SSO for the element/private/introductionScript meta data and
> when clone a SSO we would just propagate the canonical SSO.

This indeed turned out to be simpler and nicer IMO, so it's what this patch does.

Comment 6

[Jim Blandy :jimb]

It sounds to me like the ScriptSourceObject being a NativeObject at all is the source of the complexity. These are never exposed to JS directly, so letting JSScripts and Debugger.Scripts hold a reference-counted pointer to a struct with a `trace` method would be simpler. No CCWs and hence no unwrapping, but also no cloning and no need to consult a canonical SSO slot.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jim Blandy :jimb from comment #6)
> It sounds to me like the ScriptSourceObject being a NativeObject at all is
> the source of the complexity. These are never exposed to JS directly, so
> letting JSScripts and Debugger.Scripts hold a reference-counted pointer to a
> struct with a `trace` method would be simpler. No CCWs and hence no
> unwrapping, but also no cloning and no need to consult a canonical SSO slot.

Reading back the bug where SSO got added suggests there's a GC problem with this approach (bug 637572 comment 89). Maybe at some point we can assert script-cloning only happens within a Zone to avoid these issues.

I think the current patch is a clear improvement over what we have now and it unblocks the project I'm working on, so I think it's reasonable to do it for now. I should probably add a reference or summary of that comment to the SSO comment.

Comment 8

[Jim Blandy :jimb]

(In reply to Jan de Mooij [:jandem] from comment #7)
> Reading back the bug where SSO got added suggests there's a GC problem with
> this approach (bug 637572 comment 89). Maybe at some point we can assert
> script-cloning only happens within a Zone to avoid these issues.

Oh, right you are.

> I think the current patch is a clear improvement over what we have now and
> it unblocks the project I'm working on, so I think it's reasonable to do it
> for now. I should probably add a reference or summary of that comment to the
> SSO comment.

Yes, no objections to the present patch - just wondering what things should look like in the ideal world. I had forgotten that prior discussion.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Jim Blandy :jimb from comment #8)
> just wondering what things should
> look like in the ideal world. I had forgotten that prior discussion.

Thanks for bringing this up though! I added a sentence to the ScriptSourceObject SMDOC comment explaining we need SSO to properly account for cross-zone pointers for GC.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea75717477fa
Clone ScriptSourceObject when cloning scripts. r=tcampbell

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ea75717477fa

Comment 13

[Christoph Wagner]

This patch seems to have broken the Grammarly extensions for Firefox.
I'm on Firefox Developer edition x64 on Windows 10

The behavior is that clicking inspect element with closed developer tools seems to overload firefox (takes several seconds to become reactive again, highlights several elements in quick succession that the mouse pointer went over during the freeze.

Furthermore it requires several clicks in external applications (for example Outlook) to open in FF

I've run mozregression to narrow it down:

Earliest broken build:

app_name: firefox
build_date: 2018-12-16 13:06:19.156000
build_file: C:\Users\cwagner\.mozilla\mozregression\persist\ea75717477fa--autoland--target.zip
build_type: inbound
build_url: https://queue.taskcluster.net/v1/task/YgjcsGwIQCGnf5Ufx8MISA/runs/0/artifacts/public%2Fbuild%2Ftarget.zip
changeset: ea75717477fa90c798949c9dae5dfcf9ab61a2dc
pushlog_url: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f306a3db8b35c5821cbfdebd85f83b7da91822c8&tochange=ea75717477fa90c798949c9dae5dfcf9ab61a2dc
repo_name: autoland
repo_url: https://hg.mozilla.org/integration/autoland
task_id: YgjcsGwIQCGnf5Ufx8MISA

Log

2019-01-30T13:38:45: INFO : Narrowed inbound regression window from [39282f71, ea757174] (4 builds) to [c6569a81, ea757174] (2 builds) (~1 steps left)
2019-01-30T13:38:45: DEBUG : Starting merge handling...
2019-01-30T13:38:45: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=ea75717477fa90c798949c9dae5dfcf9ab61a2dc&full=1
2019-01-30T13:38:47: DEBUG : Found commit message:
Bug 1512509 - Clone ScriptSourceObject when cloning scripts. r=tcampbell

This fixes bug 1406437. It also simplifies JSScript because it now always stores
a ScriptSourceObject directly instead of a CCW for one.

Differential Revision: https://phabricator.services.mozilla.com/D13974

I'll be sending this link to the extension developers as well but thought it would be good to document it here.

Comment 14

[Jan de Mooij [:jandem]]

I'll see if I can reproduce that later. I don't see how the patch would break that, but maybe they're doing something weird.

Comment 15

[Jan de Mooij [:jandem]]

I'm unable to reproduce an observable difference in behavior before/after that change. Do you have more precise steps to reproduce?

Comment 16

[Christoph Wagner]

1. Created a new Profile
2. Installed Grammarly
3. Went to yahoo.com
4. Right-clicked somewhere and chose "Inspect Element"
5. Hovered over the elements in the developer tools, lag of page overlays occurs
6. After some time it becomes usable.
7. Close dev tools
8. Go back to step 4. problems repeats and becomes worse.

I recorded a small video showing these steps: https://i.imgur.com/D9dMXsU.mp4

Comment 17

[Jan de Mooij [:jandem]]

Christoph, that was super helpful. Thanks!

I think bug 1524565 fixes this. I'll get a Windows Try build so you can confirm.