886255 - crash in js::ion::IonFrameIterator::baselineScriptAndPc

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Cork]

This bug was filed from the Socorro interface and is 
report bp-3b254624-be8b-49e6-943b-6d1f12130624 .
============================================================= 

STR:
1. load http://paperjs.org/
2. wait for the graph animation to end

Comment 1

[Scoobidiver (away)]

The stack trace is:
Frame 	Module 	Signature 	Source
0 	libxul.so 	js::ion::IonFrameIterator::baselineScriptAndPc const 	js/src/ion/IonFrames.cpp:1579
1 	libxul.so 	js::ion::GetPcScript 	js/src/ion/IonFrames.cpp:1035
2 	libxul.so 	JSContext::currentScript const 	js/src/jscntxtinlines.h:579
3 	libxul.so 	int js::baseops::LookupProperty< 	obj-firefox/dist/include/js/RootingAPI.h:379
4 	libxul.so 	js::baseops::DeleteGeneric 	js/src/jsobj.cpp:4522
5 	libxul.so 	js::baseops::DeleteProperty 	js/src/jsobj.cpp:4566
6 	libxul.so 	JSObject::deleteProperty 	js/src/jsobjinlines.h:173
7 	libxul.so 	bool js::DeleteProperty<false> 	js/src/vm/Interpreter.cpp:3465

On Windows: bp-c0caf8f9-e341-49b8-82ad-a8b012130624.

Comment 2

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/d234e90f4d17
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130620 Firefox/24.0 ID:20130620145957
Bad:
http://hg.mozilla.org/mozilla-central/rev/b3cbafd5eb99
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130620 Firefox/24.0 ID:20130620162803
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d234e90f4d17&tochange=b3cbafd5eb99

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d1d09117491f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130620 Firefox/24.0 ID:20130620085557
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c9455df0575c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130620 Firefox/24.0 ID:20130620091228
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d1d09117491f&tochange=c9455df0575c

Regressed by:
c9455df0575c	Hannes Verschore — Bug 884310 - IonMonkey: Inline function called from .call(), r=jandem

Comment 3

[Jan de Mooij [:jandem]]

Hannes can you look into this?

Comment 4

[Hannes Verschore [:h4writer]]

Sure, already started this afternoon, but caught up with some other bugs.

Comment 5

[Hannes Verschore [:h4writer]]

This is also some Activation setActive fallout. Letting it behave even more closely to construction/destruction of JitActivation fixed it. I'll post a patch tomorrow.

Comment 6

[Tracy Walker [:tracy]]

Any progress on that patch?

Comment 7

[Hannes Verschore [:h4writer]]

Yes, I've posted it to a security bug and it has gone through first review. Since I just discovered it doesn't fix the other bug totally I will publish the patch here, soonish. I already addressed the review comments, so it should land very quickly ;)

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: Patch

Jandem: fyi you already reviewed this patch in bug 886287 . So this is the same patch with your comments addressed.

> Can you move active_ etc from Activation to JitActivation? Other activations are never inactive so it makes sense for it to be on JitActivation I think and it avoids the virtual method.

Comment 9

[Jan de Mooij [:jandem]]

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

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

::: js/src/vm/Stack.cpp
@@ +1296,2 @@
>  {
> +    if (active) {

Is this condition necessary? If it is, please add an |else| where you initialize the prevIon* fields to NULL.

@@ +1365,5 @@
>  void
>  ActivationIterator::settle()
>  {
> +    while (!done() && activation_->isJit() && !activation_->asJit()->isActive()) {
> +        if (activation_->isJit() && activation_->asJit()->isActive())

You can remove this |if| condition now (not the body), because "activation_->isJit() is part of the |while| condition.

Comment 10

[Hannes Verschore [:h4writer]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Hannes Verschore [:h4writer]]

Attachment: Pushed patch

Patch with nits addressed, carries r+ over

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 860838

User impact if declined: Crashes on said site

Testing completed (on m-c, etc.): m-i: 2 days, m-c: 1 day

Risk to taking this patch (and alternatives if risky): Risk is relative small. Not a lot of code and code behaves now more like already existing code. 

String or IDL/UUID changes made by this patch: /

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ec74c69d4177

Comment 14

[Scoobidiver (away)]

I see a crash in 25.0a1/20130709, bp-c8b5706d-19e6-412c-b40e-65a162130711, and another one in 25.0a1/20130711, bp-9c45cc97-f59c-46e8-a1a1-ea9092130711.

Comment 15

[Manuela Muntean [Away]]

I'm not able to reproduce this crash, neither with a Nightly from 2013-06-20, nor with the one from 2013-06-24.

On the other hand, I don't see a high volume of crashes in Socorro since 24.0b6, regarding last month, for both signatures:

https://crash-stats.mozilla.com/report/list?signature=js%3A%3Aion%3A%3AGetPcScript%28JSContext%2A%2C+JSScript%2A%2A%2C+unsigned+char%2A%2A%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&hang_type=any&date=2013-09-06+10%3A00%3A00&range_value=4


https://crash-stats.mozilla.com/report/list?signature=js%3A%3Aion%3A%3AIonFrameIterator%3A%3AbaselineScriptAndPc%28JSScript%2A%2A%2C+unsigned+char%2A%2A%29+const&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&hang_type=any&date=2013-09-06+10%3A00%3A00&range_value=4

Any thoughts/suggestions? Thanks!

Comment 16

[Manuela Muntean [Away]]

> I'm not able to reproduce this crash, neither with a Nightly from
> 2013-06-20, nor with the one from 2013-06-24.

I forgot to mention the OS: Win 7 64bit

Comment 17

[Robert Kaiser]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #15)
> On the other hand, I don't see a high volume of crashes in Socorro since
> 24.0b6, regarding last month, for both signatures

We recently renamed ion:: to jit:: and at least the GetPcScript signature is still around with some not-too-large frequency - but that could be something else as well:

https://crash-stats.mozilla.com/report/list?signature=js%3A%3Ajit%3A%3AGetPcScript(JSContext*%2C+JSScript**%2C+unsigned+char**)&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&hang_type=any&range_value=4

Comment 18

[Manuela Muntean [Away]]

> We recently renamed ion:: to jit:: and at least the GetPcScript signature is
> still around with some not-too-large frequency - but that could be something
> else as well:
> 
> https://crash-stats.mozilla.com/report/
> list?signature=js%3A%3Ajit%3A%3AGetPcScript(JSContext*%2C+JSScript**%2C+unsig
> ned+char**)&product=Firefox&query_type=contains&range_unit=weeks&process_type
> =any&hang_type=any&range_value=4

There are only 17 crashes with 24.0b7 and 4 with 24.0b8,so I'm marking this verified as fixed, based on comment 16 and comment 17.

Comment 19

[Manuela Muntean [Away]]

> There are only 17 crashes with 24.0b7 and 4 with 24.0b8,so I'm marking this
> verified as fixed, based on comment 16 and comment 17.

And comment 15.

Comment 20

[u279076]

Verified fixed based on lack of instances of this signature in crash-stats for Firefox 25+.