1229493 - crash in js::CrossCompartmentKey::CrossCompartmentKey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[RealDolos]

Attachment: 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.

Comment 1

[Loic]

Recent regression.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=793916ec6dd4c589a8d589967158ccff532527ac&tochange=0773712473c9cea41fa3a063f97cbd2dc55d86a4

Comment 2

[Loic]

FF45 CR:
https://crash-stats.mozilla.com/report/index/22d1ecf3-d5e8-4a53-bef9-2727c2151202

Comment 3

[Jon Coppeard (:jonco)]

Attachment: firefox-modules-crash

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.

Comment 4

[Shu-yu Guo [: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?

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9865d59bbb8

Comment 6

[Jon Coppeard (:jonco)]

(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.

Comment 7

[Jon Coppeard (:jonco)]

Needinfo'ing myself as a reminder to request uplift once this has baked on central for a couple of days.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 43.

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Jon Coppeard (:jonco)]

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.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 13

[Loic]

(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.

Comment 14

[Jon Coppeard (:jonco)]

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.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 18

[Jon Coppeard (:jonco)]

Attachment: bug1229493-beta-patch

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.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e7aa47e75e67

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e7aa47e75e67