752098 - Crashes on MemBench with incremental GC enabled

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Gregor Wagner [:gwagner]]

This bug was filed from the Socorro interface and is 
report bp-51004ce3-ccbe-4451-90f0-237052120504 .
============================================================= 

Just start http://gregor-wagner.com/tmp/mem and wait a few sec.

Comment 1

[Gregor Wagner [:gwagner]]

I also see this signatures:
https://crash-stats.mozilla.com/report/index/bp-a1df900b-2ad0-4d2f-9c30-d026c2120504
 https://crash-stats.mozilla.com/report/index/bp-8486489e-8582-47f9-b055-6b3692120504

I hope that's just one bug....

Comment 2

[Gregor Wagner [:gwagner]]

Important detail: IGC is enabled.
Seems stable without IGC.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I just tried this with a debug Mac build from changeset 40a901a6e733.  No crash....

With igc, is it possible for an object to be finalized more than once?  I'd assume not, right?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> With igc, is it possible for an object to be finalized more than once?  I'd
> assume not, right?

They're only called once.

The second crash seems to be bug 752081.

Comment 5

[Gregor Wagner [:gwagner]]

A few other signatures I see:
https://crash-stats.mozilla.com/report/index/bp-14cd6291-a85e-462a-9231-5490a2120505
https://crash-stats.mozilla.com/report/index/bp-ed708741-4f91-4f0f-9b89-d264d2120505
https://crash-stats.mozilla.com/report/index/bp-3dd410d9-c428-47da-af69-91a462120505
https://crash-stats.mozilla.com/report/index/bp-b27d692a-2c0a-4ba7-9327-811422120505
https://crash-stats.mozilla.com/report/index/bp-337e32f3-7627-40b5-aa39-0f06e2120505

Comment 7

[Andrew McCreight [:mccr8]]

Danial Horton narrowed down the range a bit, but there are around 200 changesets in the range he found, so it is still fairly wide.

Comment 8

[Andrew McCreight [:mccr8]]

I got a similar crash just now with incremental enabled.  Just on Google Reader.

https://crash-stats.mozilla.com/report/index/bp-77c96c0e-9f96-4bc9-bef8-198572120505

Comment 9

[Danial Horton]

if we both started seeing it roughly around the same changesets, then it could be the same thing, especially if his tests was on inbound where the CPG and other stuff was already landed.

for me, 20120503142649 is the last build i could open my usual session on with iGC enabled.

my DSL is flaky at nights atm, which makes diving into the inbound builds a problem.(In reply to Andrew McCreight [:mccr8] from comment #8)

> I got a similar crash just now with incremental enabled.  Just on Google
> Reader.
> 
> https://crash-stats.mozilla.com/report/index/bp-77c96c0e-9f96-4bc9-bef8-
> 198572120505

Yeah, thats pretty much exactly what i had been seeing
I was just using membench in the bug i opened to exacerbate the situation and bring a crash on quicker.

In some cases, all i have to do is merely interact with the ui, even as slight as minimising the window and it might crash while iGC is on.

Comment 10

[Andrew McCreight [:mccr8]]

Yeah, this is pretty terrible.  I'm going to disable incremental GC for now.  There are one or two other reports of crashy IGC in bug 654903.

Comment 11

[Danial Horton]

alright, i just dropped my sync speed a bit, i'll get a few inbound zips downloading and set up a profile with igc enabled to run through membench till i find where it stops crashing

Comment 12

[Andrew McCreight [:mccr8]]

Well, there's already one patch in that range that is known to have caused a regression (bug 730208), and there's a patch to fix it, but it just hasn't landed yet.  Probably a good place to start would be to look at builds before that landed.

Comment 13

[Danial Horton]

Regression window(m-i)

Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c3813fbb1c9a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1
ID:20120502040217

Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/de5745bce8bc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1
ID:20120503040220

Comment 14

[Danial Horton]

installed memchaser and nightly tools to be sure of my findings... which had the effect of making the regression appear quicker in 20120503040220

Comment 15

[Danial Horton]

This doesn't appear to be osx only, so platform might do with changing to All All?

now all we need is someone to test iGC on a droid and see if it happens there for a trifecta....

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'll work on this soon. It sounds like it's affecting a lot of people.

Comment 17

[Danial Horton]

You sure Andrew?, that crash seems to predate this particular regression.

Comment 18

[Andrew McCreight [:mccr8]]

Bug 654903 likely has many causes.  This bug is one of them, for people who have incremental GC turned on.

Comment 19

[Danial Horton]

initial report in bug 654903 is not likely to be caused by incremental GC at all, the last 4 or so comments in it are related to this issue, but that doesn't mean the overall bug is.

I remember getting that particular crash earlier last year though and thought it had since been resolved (something specific to JIT and certain extensions?)

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

Thanks for the bisection, Daniel. It helped a lot.

The main problem is that, while the gcIsFull flag is set during BeginMarkPhase, we're using it during the sweep phase. So it's possible for a compartment to be created in between and not have its script filenames marked, but it will get them swept.

One possible solution is to change the gcIsFull flag so that it's set during sweeping. But then we might set mark bits but not sweep, which is sort of bad, although only theoretically. Generally I think it's better to set flags like this in the first slice and then leave them alone. Otherwise it gets really confusing what should be marked.

The solution I've chosen is to ensure that all new script filenames are marked. This includes ones created for new compartments.

The right solution for this is bug 751618.

Comment 22

[Luke Wagner [:luke]]

Comment on attachment 621785 [details] [diff] [review]
patch

Thanks to you both!

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 24

[Danial Horton]

Ok so Lukes patch is a temporary fix till bug 751618 is looked into and implemented? (coz if thats the full fix, it just did my head in... a single line... xD)

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Danial Horton from comment #24)
> Ok so Lukes patch is a temporary fix till bug 751618 is looked into and
> implemented? (coz if thats the full fix, it just did my head in... a single
> line... xD)

This is a real fix for the problem. Bug 751618 would just solve it in a cleaner way. Sometimes scary bugs have simple fixes :-).

Comment 26

[Danial Horton]

be nice if 20120507 could get a respin just for this, understandably we don't always get nice things :-(

i'll check out the hourly if i remember. thanks for the quick fix guys