Closed Bug 1363301 Opened 3 years ago Closed 2 years ago

Crash in js::TraceLoggerEvent::TraceLoggerEvent

Categories

(Core :: XPConnect, defect, P2, critical)

55 Branch
Unspecified
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 + fixed
firefox56 --- fixed

People

(Reporter: calixte, Assigned: kmag)

References

(Blocks 1 open bug)

Details

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

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-7c8664af-74db-4c27-b1a4-3716b0170508.
=============================================================

There are 8 crashes in nightly 55 with buildid 20170508030204. In analyzing the backtrace the regression may have been introduced by patch [1] to fix bug 1359653.
The url for these crashes is "about::addons".

[1] https://hg.mozilla.org/mozilla-central/rev?node=4303fccdaea09a9acf0d72d03e1d3060e20d43ca
Flags: needinfo?(kmaglione+bmo)
I crashed after removing an addon from about:addons and clicking on "cancel" to bring it back.
[Tracking Requested - why for this release]:

I crashed too after opening the console on about:addons to check some errors about an add-on:
https://crash-stats.mozilla.com/report/index/cbd5a234-33e8-4991-af07-566320170517
Tracking for 55 as new regression.
Can you help find someone to investigate the crash here? Thanks. I'll also email kmag in case he has suggestions or missed the n-i.
Flags: needinfo?(overholt)
Kris would know more here than I do, sorry.
Flags: needinfo?(overholt)
It's certainly possible that this could have been caused by bug 1359653, but I'm not sure how likely it is. Those patches generally only affect scripts that are loaded at startup, so they're not likely to affect scripts loaded after startup unless they're loaded very early, and then loaded again later. That's certainly a possibility, though.

Beyond that, looking at the stacks, it seems most likely that we're winding up with a null or otherwise invalid scriptSource property for the scripts we're executing. If anyone can reproduce this consistently, particularly with a debug build, and can inspect it with a debugger, that would help track it down.
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P2
STR:
1. Install example legacy XPI extension: 
https://github.com/mdn/webextensions-examples/tree/master/embedded-webextension-sdk/step0-legacy-addon
extensions.allow-non-mpc-extensions = true
xpinstall.signatures.required = false
2. Restart browser.
3. Disable extension.
4. Enable extension.

Usually crashes first time but may take another attempt.
The WebExtension version of the example addon does not crash with the above steps.
Attached file sdk-addon-name.xpi
i'm able to reproduce the crash with the str in comment #7 (at least when disabling/reenabling a couple of times in quick succession): bp-74c9ff12-11fb-4162-aa52-6c89b0170614
attaching the xpi for convenience...

55.0b1 builds were just released a couple of hours ago, but early crash data suggests that this signature might become a top crasher on beta.
Flags: needinfo?(kmaglione+bmo)
Thanks for the STR. I managed to reproduce this in a debug build, and it looks like sourceObject_ for this script is somehow winding up as a dead wrapper, which means this is similar to bug 1348666.

Since we no longer nuke wrappers for ScriptSourceObjects, I guess in this case we're failing to create a new wrapper for a ScriptSourceObject that already lives in a nuked sandbox compartment (for the resource://gre/modules/commonjs/sdk/addon/runner.js script). And that can only happen when 1) the script was compiled and cached for the first time during startup for the current session, b) the sandbox that it was compiled for was nuked during this session, and c) the startup cache hasn't been flushed in the mean time.

So I'm kind of surprised we're seeing it in the wild at all.
Flags: needinfo?(kmaglione+bmo)
Assignee: nobody → kmaglione+bmo
Hm, do we really need to do this? It seems to me like this change basically allows globals to be held alive via these script source references. It seems like it would be better for the consumers of those references to type-check their referents and throw if they're dead objects...
We only get into this situation in a rare enough corner cases that I'm not too worried about globals being held alive. Essentially, we have to compile a script during startup, store it in the startup cache, nuke the global it was originally compiled for, and then load it again without flushing the cache. Which basically means exactly following the steps from comment 7.

Shu mentioned at one point wanting to stop using wrappers for ScriptSourceObjects, since they're not meant to be tied to compartments, but I don't have the relevant context to work on that, or the time right now gain that context.

The only other option I have is to always compile subscript loader scripts for the compilation scope at startup, and then clone them into the target scope. But in that case, we'd still need to add assertions that we're not creating dead wrappers for ScriptSourceObjects, so given the rarity of this corner case, I don't think there's much to gain.
Comment on attachment 8877782 [details]
Bug 1363301: Always provide live wrappers for ScriptSourceObjects.

https://reviewboard.mozilla.org/r/149210/#review153734

I guess it seems to me that we shouldn't be caching scripts that were compiled in a compartment that might be nuked. But I guess the point is that we don't control that? Anyway, I'll defer to your judgement here.
Attachment #8877782 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #14)
> I guess it seems to me that we shouldn't be caching scripts that were
> compiled in a compartment that might be nuked. But I guess the point is that
> we don't control that? Anyway, I'll defer to your judgement here.

In this case, we're compiling the scripts for SDK module sandboxes, and we need to cache them so they can be stored in the next session's bytecode cache and preloaded at startup.

In general, that doesn't cause any issues, because we only compile the scripts in the Sandbox compartment the first startup after the extension is installed (after that, they're pre-compiled in the compilation scope), we only nuke those sandboxes when the extension is shut down, and when the extension is uninstalled or upgraded, we flush the cache immediately after those compartments are nuked.

So the only time it can ever be an issue is when the extension is disabled and re-enabled the next startup after it's installed.
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #15)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #14)
> > I guess it seems to me that we shouldn't be caching scripts that were
> > compiled in a compartment that might be nuked. But I guess the point is that
> > we don't control that? Anyway, I'll defer to your judgement here.
> 
> In this case, we're compiling the scripts for SDK module sandboxes, and we
> need to cache them so they can be stored in the next session's bytecode
> cache and preloaded at startup.
> 
> In general, that doesn't cause any issues, because we only compile the
> scripts in the Sandbox compartment the first startup after the extension is
> installed (after that, they're pre-compiled in the compilation scope), we
> only nuke those sandboxes when the extension is shut down, and when the
> extension is uninstalled or upgraded, we flush the cache immediately after
> those compartments are nuked.
> 
> So the only time it can ever be an issue is when the extension is disabled
> and re-enabled the next startup after it's installed.

Ok - might be worth putting this reasoning in a comment somewhere.
Comment on attachment 8877782 [details]
Bug 1363301: Always provide live wrappers for ScriptSourceObjects.

https://reviewboard.mozilla.org/r/149210/#review154140
Attachment #8877782 - Flags: review?(shu) → review+
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #16)
> Ok - might be worth putting this reasoning in a comment somewhere.

Added a comment to the subscript loader where the cached script is saved.
Comment on attachment 8877782 [details]
Bug 1363301: Always provide live wrappers for ScriptSourceObjects.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1359653
[User impact if declined]: This patch fixes a crash.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: STR is in comment 7.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low-risk.
[Why is the change risky/not risky?]: This is a relatively minor change that adds an exception to some logic which is intended to prevent certain kinds of leaks. The leak-prevention logic is relatively new, so this change restores older behavior in certain circumstances, and we have a similar exception for this type of object in other places.
[String changes made/needed]: None.
Attachment #8877782 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/02383934eea2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8877782 [details]
Bug 1363301: Always provide live wrappers for ScriptSourceObjects.

crash fix, beta55+

Should be in 55.0b3
Attachment #8877782 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.