Closed
Bug 1398499
Opened 7 years ago
Closed 7 years ago
Crash in ExecuteInNonSyntacticGlobalInternal
Categories
(Core :: XPConnect, defect)
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)
Comment 1•7 years ago
|
||
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.
Comment 2•7 years ago
|
||
It's possible some people were watching bug 1186409 and decided to flip the pref for testing.
Comment 3•7 years ago
|
||
Kris might have some idea what is going wrong.
Flags: needinfo?(kmaglione+bmo)
Comment 4•7 years ago
|
||
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.
Comment 5•7 years ago
|
||
(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.
Comment 6•7 years ago
|
||
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.
Comment 7•7 years ago
|
||
(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.
Comment 8•7 years ago
|
||
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)
Assignee | ||
Comment 9•7 years ago
|
||
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)
Comment 10•7 years ago
|
||
I was imagining something simple like this. We need a test case for it too.
Comment 11•7 years ago
|
||
Oops.. that patch is broken, but that is the idea.
Assignee | ||
Comment 12•7 years ago
|
||
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 :(
Assignee | ||
Comment 13•7 years ago
|
||
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.
Updated•7 years ago
|
Attachment #8906306 -
Attachment is obsolete: true
Comment 14•7 years ago
|
||
The path naming sounds like the right call. I'll leave it to you and Andrew.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → kmaglione+bmo
Component: JavaScript Engine → XPConnect
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
mozreview-review |
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 19•7 years ago
|
||
mozreview-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 20•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 21•7 years ago
|
||
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
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dab1c414753a https://hg.mozilla.org/mozilla-central/rev/f8528dbd0e70 https://hg.mozilla.org/mozilla-central/rev/952de83710f9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 23•7 years ago
|
||
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?
Reporter | ||
Updated•7 years ago
|
Crash Signature: [@ ExecuteInNonSyntacticGlobalInternal] → [@ ExecuteInNonSyntacticGlobalInternal]
[@ ExecuteInNonSyntacticGlobalInternal [clone .cold.607]]
Comment 24•7 years ago
|
||
(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.
Assignee | ||
Comment 25•7 years ago
|
||
mozreview-review |
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.
Comment 26•7 years ago
|
||
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.
Description
•