Closed Bug 1398499 Opened 7 years ago Closed 7 years ago

Crash in ExecuteInNonSyntacticGlobalInternal

Categories

(Core :: XPConnect, defect)

57 Branch
Unspecified
Windows 10
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: calixte, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [clouseau])

Crash Data

Attachments

(3 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-ca3d8f62-dd59-488e-96a0-5a2690170909.
=============================================================

There are 6 crashes (from 2 installs) in nightly 57 with buildid 20170909100226. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1395360.

[1] https://hg.mozilla.org/mozilla-central/rev?node=d731723c09f704c5063140644b5916615261307e
Flags: needinfo?(tcampbell)
This is more strictly a regression from bug 1186409. I'm not sure how somebody is hitting this code. It shouldn't happen with the pref disabled.
Blocks: 1186409
No longer blocks: 1395360
It's possible some people were watching bug 1186409 and decided to flip the pref for testing.
Kris might have some idea what is going wrong.
Flags: needinfo?(kmaglione+bmo)
I'll take a look this afternoon. I think Emanuel is right that this is a dev flipping the pref. Either way, we shouldn't be crashing.

The crash is that somehow we are compiling a script without nonsyntactic environment support, but then trying to execute with it.
(In reply to Ted Campbell [:tcampbell] from comment #4)
> The crash is that somehow we are compiling a script without nonsyntactic
> environment support, but then trying to execute with it.

Ah, that makes sense. I could imagine that the preloader has cached a version that was compiled with the pref set to off, then somebody set it to true. The other code paths seem ok.
That does match up with the STR in bug 1186409 comment 158. We're up to 54 crashes across 6 users now. One quick "fix" would be to hard disable this to ignore the pref. Although users who set the pref may get the opposite problem, where a script compiled with the pref enabled in the preloader would get loaded into a state where it is disabled, so I guess I need to figure out how to deal with that anyways.
(In reply to Andrew McCreight [:mccr8] from comment #6)
> That does match up with the STR in bug 1186409 comment 158. We're up to 54
> crashes across 6 users now.
That must be my crashes. I've posted improved STR here: https://bugzilla.mozilla.org/show_bug.cgi?id=1186409#c159
Also, Nightly in my regular profile has somehow cured itself and it no longer crashes with jsloader.shareGlobal  true and dom.ipc.processCount 7 after I restarted it a couple of times.
Thanks for reporting! I think Andrew is on to something with the caching theory. The quick fix might be to check the flag after loading from startup cache and if it isn't expected value, continue with normal cold load.
Flags: needinfo?(tcampbell)
My guess is that we're storing the global-compiled version in the startup cache, then someone is enabling the shareGlobal pref and restarting.

For normal deployment, this wouldn't be a problem, because we flush the startup cache after the build ID changes. But after a simple pref flip...

Maybe we should just use a different cache path for non-syntactic vs. global compiled scripts.
Flags: needinfo?(kmaglione+bmo)
I was imagining something simple like this. We need a test case for it too.
Oops.. that patch is broken, but that is the idea.
I guess that would work, but I was thinking something along the lines of:

    cachePath.AppendLiteral(shareGlobal ? "/non-syntactic" : "/global");

just before PathifyURI. I guess either approach would have approximately the same effect.

The test would probably need to be a Marionette test :(
Hm. No, I take that back. We only generate XDR data for the preloader cache if we don't already have an entry from the previous session, so if we don't change the path, we'll wind up re-compiling the script at every startup until the cache is flushed.
Attachment #8906306 - Attachment is obsolete: true
The path naming sounds like the right call. I'll leave it to you and Andrew.
Assignee: nobody → kmaglione+bmo
Component: JavaScript Engine → XPConnect
Comment on attachment 8906331 [details]
Bug 1398499: Part 3 - Add Marionette tests for global sharing.

https://reviewboard.mozilla.org/r/178062/#review183000

::: commit-message-2c3a0:3
(Diff revision 1)
> +Bug 1398499: Part 3 - Add Marionette tests for global sharing. r?mccr8
> +
> +This tests both that the settings have the desired affect and that switching

nit: effect
Attachment #8906331 - Flags: review?(continuation) → review+
Comment on attachment 8906330 [details]
Bug 1398499: Part 2 - Add MOZ_LOADER_SHARE_GLOBAL env var to toggle global sharing.

https://reviewboard.mozilla.org/r/178060/#review183002
Attachment #8906330 - Flags: review?(continuation) → review+
Comment on attachment 8906329 [details]
Bug 1398499: Part 1 - Use separate cache paths for shared/unshared scripts.

https://reviewboard.mozilla.org/r/178058/#review183004

Thanks for fixing this and writing tests.
Attachment #8906329 - Flags: review?(continuation) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/dab1c414753a9d69e7d753f114a9d06cd2c08d8b
Bug 1398499: Part 1 - Use separate cache paths for shared/unshared scripts. r=mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8528dbd0e708bc0f26dd4823ef8ee113c03636c
Bug 1398499: Part 2 - Add MOZ_LOADER_SHARE_GLOBAL env var to toggle global sharing. r=mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/952de83710f9b3a9757916cbe77081d9d08a7b85
Bug 1398499: Part 3 - Add Marionette tests for global sharing. r=mccr8
I set jsloader.shareGlobal to true in my regular Nightly profile yesterday. Should I do anything else in order to make sure everything works as intended, especially after jsloader.shareGlobal is enabled by default?
Crash Signature: [@ ExecuteInNonSyntacticGlobalInternal] → [@ ExecuteInNonSyntacticGlobalInternal] [@ ExecuteInNonSyntacticGlobalInternal [clone .cold.607]]
(In reply to ajfhajf from comment #23)
> I set jsloader.shareGlobal to true in my regular Nightly profile yesterday.
> Should I do anything else in order to make sure everything works as
> intended, especially after jsloader.shareGlobal is enabled by default?

No, it should be fine.
Comment on attachment 8906331 [details]
Bug 1398499: Part 3 - Add Marionette tests for global sharing.

https://reviewboard.mozilla.org/r/178062/#review183036

::: js/xpconnect/tests/marionette/manifest.ini:2
(Diff revision 1)
> +
> +[test_loader_global_sharing.py]

Note: I had to disable this on Android, because we can't restart the browser there, and add an [include:] to the central Marionette unit-tests.ini, because apparently Marionette tests are even weirder than I realized.
I don't see any of this crash on the 9-10 builds.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.