Closed Bug 1229493 Opened 4 years ago Closed 4 years ago

crash in js::CrossCompartmentKey::CrossCompartmentKey

Categories

(Core :: JavaScript Engine, defect, critical)

43 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- unaffected
firefox43 + wontfix
firefox44 + fixed
firefox45 + fixed
b2g-v2.5 --- fixed

People

(Reporter: u557094, Assigned: jonco)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files)

Attached file volapg.0.4.user.js
This bug was filed from the Socorro interface and is 
report bp-9e630ceb-7253-40dd-9e8f-8ea882151201.
=============================================================

- install Greasemonkey (restart)
- install user script from attachment
- goto to https://volafile.io/r/404 (does not exist, but triggers user script)
--> CRASH

Even if the script was buggy it should not crash the entire browser.
Works in 42 Release therefore a regression in 43 Beta.
Keywords: regression
Recent regression.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=793916ec6dd4c589a8d589967158ccff532527ac&tochange=0773712473c9cea41fa3a063f97cbd2dc55d86a4
Blocks: 930414
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash
Hardware: Unspecified → All
Version: unspecified → 43 Branch
FF45 CR:
https://crash-stats.mozilla.com/report/index/22d1ecf3-d5e8-4a53-bef9-2727c2151202
Crash Signature: [@ js::CrossCompartmentKey::CrossCompartmentKey] → [@ js::CrossCompartmentKey::CrossCompartmentKey] [@ JSCompartment::wrap ]
Assignee: nobody → jcoppeard
The problem here is that I gave the module classes cached prototypes and added them to the list of classes in jsprototypes.h.  This effectively made them into standard classes and made their presence visible outside of the shell which was not intended.

In this case XrayTraits::resolveOwnProperty() tries to lookup the name 'Module' on a global object and decides that it's a standard class because JS_IdToProtoKey() returns a valid key for that id.  Then it tries to get its constructor, but this has not been set and we crash (or assert in debug builds).

Instead, we should store the prototypes for these classes elsewhere on the global as we do in other cases.  The patch eagerly creates them when a shell global is created.  This doesn't happen lazily as is the case for most other prototypes since we will want to create objects of these classes off main thread and would otherwise have to synchronise when initialising the prototype slots on the global.

I verified that this fixed the crash.
Attachment #8694816 - Flags: review?(shu)
Comment on attachment 8694816 [details] [diff] [review]
firefox-modules-crash

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

This looks fine to me. I wonder if it regresses any memory benchmarks though.

::: js/src/builtin/ModuleObject.cpp
@@ +541,5 @@
>      nullptr,        /* setProperty */
>      nullptr,        /* enumerate   */
>      nullptr,        /* resolve     */
>      nullptr,        /* mayResolve  */
> +    ModuleObject::finalize,

o wow was it just never set before?
Attachment #8694816 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #4)
> This looks fine to me. I wonder if it regresses any memory benchmarks though.

Should be fine, but I guess we'll find out.

> o wow was it just never set before?

Yeah, I just spotted it when fixing this.
Needinfo'ing myself as a reminder to request uplift once this has baked on central for a couple of days.
Flags: needinfo?(jcoppeard)
While this does not show a huge # of crashes on 44.0a2, given that a patch is almost ready, I will track for 44 and hopefully uplift to Aurora44/Beta44 soon.
https://hg.mozilla.org/mozilla-central/rev/c9865d59bbb8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8694816 [details] [diff] [review]
firefox-modules-crash

Approval Request Comment
[Feature/regressing bug #]: Bug 930414.
[User impact if declined]: Possible browser crashes.
[Describe test coverage new/current, TreeHerder]: On central since 4th December.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8694816 - Flags: approval-mozilla-aurora?
Jon, while reviewing the aurora44 (new beta44) uplift request, I noticed that this crash still occurred on FF45 (45.0a1  BuildId: 20151213030241). Perhaps, the crash isn't really fixed. Thoughts? Is it still worth uplifting as is? Please let me know.
Flags: needinfo?(jcoppeard)
Blocks: 1233638
(In reply to Ritu Kothari (:ritu) from comment #12)
> Jon, while reviewing the aurora44 (new beta44) uplift request, I noticed
> that this crash still occurred on FF45 (45.0a1  BuildId: 20151213030241).
> Perhaps, the crash isn't really fixed. Thoughts? Is it still worth uplifting
> as is? Please let me know.

IMHO, the crash signatures for this bug are generic, so FF45 is still crashing with the same signature for different reasons.
Yes I think this is still worth uplifting.

I confirmed that the fix is present on central and on the aurora branch, and that nightly does not crash anymore with these STR.  Developer edition seems to be version 44 still so I couldn't test that unfortunately.  I think what you're seeing must be as Loic said a different crash.
Flags: needinfo?(jcoppeard)
No longer blocks: 1233638
Duplicate of this bug: 1233638
Duplicate of this bug: 1233636
Terrence, Jon: Could you please nominate a patch for uplift to Beta44 and Aurora45 soon? I would like to include this fix in 44.0b2 (pushed to beta channel on Tuesday) if possible.
Flags: needinfo?(terrence)
Flags: needinfo?(jcoppeard)
Here's a patch for beta.  The fix has already merged to aurora.

Tested locally and pushed a try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb5ab071193b

Approval Request Comment
[Feature/regressing bug #]: Bug 930414.
[User impact if declined]: Possible browser crashes.
[Describe test coverage new/current, TreeHerder]: On central since 4th December.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Flags: needinfo?(terrence)
Flags: needinfo?(jcoppeard)
Attachment #8700577 - Flags: review+
Attachment #8700577 - Flags: approval-mozilla-beta?
Attachment #8694816 - Flags: approval-mozilla-aurora?
Comment on attachment 8700577 [details] [diff] [review]
bug1229493-beta-patch

This is a crash fix, that has been verified and has stabilized in Nightly for over 2 weeks, seems safe to uplift to Beta44.
Attachment #8700577 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.