1115776 - Crashes in EnterIon on Pinterest

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

This bug was filed from the Socorro interface and is 
report bp-7084ac9f-b873-4608-adeb-159db2141219.
=============================================================
This is currently the #6 topcrash for Firefox 34.0.5 with 4467/162224 crashes in the last 7 days. It's in the top 10 crash signatures for 34.0 as well. 

Many comments mention Pinterest. 



crashing thread:

0 		@0xabe23b7 	
1 	mozjs.dll 	EnterIon 	js/src/jit/Ion.cpp
2 	mozjs.dll 	js::jit::IonCannon(JSContext*, js::RunState&) 	js/src/jit/Ion.cpp
3 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp
4 	mozjs.dll 	CanEnterBaselineJIT 	js/src/jit/BaselineJIT.cpp
5 	mozjs.dll 	js::InterpreterFrame::epilogue(JSContext*) 	js/src/vm/Stack.cpp
6 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp
7 	mozjs.dll 	JSObject::putProperty<0>(js::ExclusiveContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>), bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>), unsigned int, unsigned int, unsigned int) 	js/src/vm/Shape.cpp
8 	mozjs.dll 	DefinePropertyOrElement<0> 	js/src/jsobj.cpp
9 	mozjs.dll 	js::GetElements(JSContext*, JS::Handle<JSObject*>, unsigned int, JS::Value*) 	js/src/jsarray.cpp
10 	mozjs.dll 	mozjs.dll@0x2033ff 	
11 		@0x17d78f3f 	
12 	mozjs.dll 	js::types::TypeObject::markPropertyNonData(js::ExclusiveContext*, jsid) 	js/src/jsinfer.cpp
13 	mozjs.dll 	js::types::MarkTypePropertyNonData(js::ExclusiveContext*, JSObject*, jsid) 	js/src/jsinferinlines.h
14 	mozjs.dll 	JSObject::changeProperty<0>(js::ExclusiveContext*, JS::Handle<JSObject*>, JS::Handle<js::Shape*>, unsigned int, unsigned int, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>), bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>)) 	js/src/vm/Shape.cpp
15 	mozjs.dll 	js::jit::GetPcScript(JSContext*, JSScript**, unsigned char**) 	js/src/jit/IonFrames.cpp
16 	mozjs.dll 	js::types::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&)

Comment 1

[(Away)]

I suspect EnterIon is an aggregate of many different crashes...

Jan, what information could we dig up to make this bug more actionable?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to dmajor (away) from comment #1)
> Jan, what information could we dig up to make this bug more actionable?

I've mentioned this a few times but it'd be really useful to have a disassembly view in crash-stats, probably behind the login. I currently use Ted's tool for this but it takes a lot more time. Once we have that, it should be easier to categorize them.

Other than that I don't know. Many comments/URLs mention pinterest (and the addon), I'd like to look into that again today.

Comment 3

[Jan de Mooij [:jandem]]

I do hit the crash on pinterest sometimes... Not reliably but hopefully I can add some instrumentation.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

I was able to track down the pinterest crash. For LApplyArgsGeneric, we didn't call branchIfFunctionHasNoScript if there's a single callee, but this is wrong because functions can be relazified now.

This patch just always calls branchIfFunctionHasNoScript and should be very safe to backport.

Tomorrow I want to investigate how we can trigger this in the shell; it'd be nice if the fuzzers could find those bugs more easily.

Comment 5

[Jan de Mooij [:jandem]]

[Tracking Requested - why for this release]:
Topcrash and safe fix, we should backport this.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8542774 [details] [diff] [review]
Patch

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

LGTM.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8542774 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not trivial, maybe with some work.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
Assuming 29+.

> If not all supported branches, which bug introduced the flaw?
Bug 886193, most likely.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Very easy to backport.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely.

Approval Request Comment
[Feature/regressing bug #]: Bug 886193 most likely.
[User impact if declined]: Topcrash on Pinterest and likely other websites. Security issues.
[Describe test coverage new/current, TBPL]: Confirmed it fixes the pinterest crash.
[Risks and why]: Low risk. Very small and safe patch.
[String/UUID change made/needed]: None.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Shell testcase

Crashes JS shell without the patch.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> Crashes JS shell without the patch.

May require --ion-offthread-compile=off / --no-threads.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

This is too late to make it into Firefox 35. We're out of Betas for that and the next build is the RC.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

The question about sec-approval, since we missed the Beta branch, is how hard it is to figure out an exploit from this patch. I'd like to approve this now but I'm afraid of having it everywhere but 35 for six weeks.

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Al Billings [:abillings] from comment #10)
> This is too late to make it into Firefox 35. We're out of Betas for that and
> the next build is the RC.

That's pretty unfortunate but I guess it can wait another cycle..

(In reply to Al Billings [:abillings] from comment #11)
> The question about sec-approval, since we missed the Beta branch, is how
> hard it is to figure out an exploit from this patch. I'd like to approve
> this now but I'm afraid of having it everywhere but 35 for six weeks.

Maybe we should just land this everywhere a few weeks from now?

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Topcrash, tracking

Comment 14

[Robert Kaiser]

I don't really agree with Al's comment #10 here. This has been rising somewhat to now #13 with .8% of all crashes on 35.0b8, and it is a sec-crit, happening on very common websites (Pinterest at least). I fear that having it sit around for 8+ weeks until we ship a fix is not a good idea, esp. as we have a patch in hand that is categorized as "low risk, very small and safe". This is the kind of thing I think we can even take on RC, despite me being pretty loud recently (and that will continue) on making fewer uplifts in general, esp. to beta.

BTW, on 34.0.5 release, this is now #5 with 1.3% of all crashes, so it impacts release users about double as much as Beta users.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Robert, we have no betas and are about to build our RC. We aren't taking this for 35, in my opinion. I'll needinfo? Release Management folks but I doubt they are going to approve it.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8542774 [details] [diff] [review]
Patch

sec-approval+ for checkin on Jan 26.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

If waiting is an option, happy to wait.  Thanks for the ping.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Looks like this needs rebasing on trunk.

Comment 19

[Jan de Mooij [:jandem]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #18)
> Looks like this needs rebasing on trunk.

https://hg.mozilla.org/integration/mozilla-inbound/rev/68e641adcfb6

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/936d24a4d78a

Needs rebasing for beta.

Comment 21

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9ac8ce8d36ef

Keeping NI for ESR31.

Comment 22

[Jan de Mooij [:jandem]]

And ESR31:

https://hg.mozilla.org/releases/mozilla-esr31/rev/95e4252b6d74

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68e641adcfb6
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/936d24a4d78a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f37f23eb2ada
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/09b7b1f86aba
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/8d2892d26aa8

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/f37f23eb2ada
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/09b7b1f86aba

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'EnterIon':
 - nightly (version 50): 2 crashes from 2016-06-06.
 - aurora  (version 49): 3 crashes from 2016-06-07.
 - beta    (version 48): 16 crashes from 2016-06-06.
 - release (version 47): 67 crashes from 2016-05-31.
 - esr     (version 45): 12 crashes from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          0          0          0          0          0          0          0
 - aurora           0          0          1          0          0          0          0
 - beta             2          2          2          2          6          0          0
 - release         12         10          8          9         13          9          5
 - esr              0          1          1          3          1          2          1

Affected platform: Windows