1275190 - Crash in js::SavedFrame::HashPolicy::match

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

This bug was filed from the Socorro interface and is 
report bp-07cd7453-4e60-42af-80bc-553e32160524.
=============================================================

Null deref crash, has only occurred once. fitzgen, is it possible that |existing| is null here?

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> This bug was filed from the Socorro interface and is 
> report bp-07cd7453-4e60-42af-80bc-553e32160524.
> =============================================================
> 
> Null deref crash, has only occurred once. fitzgen, is it possible that
> |existing| is null here?

It would be very very strange if it was null, but at the least we should add some more asserts.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hm... The only place we insert into this table has a null check of the value being inserted directly above it: https://dxr.mozilla.org/mozilla-central/rev/46fe2115d46a5bb40523b8466341d8f9a26e1bdf/js/src/vm/SavedStacks.cpp#1371

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Assert against null entries in the SavedFrame hash set's hash policy

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f25b2495f5b

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Assert that the SavedFrame* pointers we get out of js::SavedStacks::frames are not null

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Cancelled old try push, here is a new one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=584348d33bf0

Comment 7

[Pulsebot]

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

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Only one of the two patches was checked in previously. The leave-open is because these patches don't fix any crash, are just diagnostic.

Comment 9

[Pulsebot]

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

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for likely breaking browser_perf-recordings-clear-02.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/65806002153d

Push range with last good and first bad: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&autoclassify&fromchange=26b2ba6d410853e03239e684c6255fc44252b1c1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&filter-searchStr=888715a6f2f6acfe351d1fa6b3c08251dd70a039&tochange=10db03f821c4a9bf76d1e516c32eb3c9317aed27
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=28748516&repo=mozilla-inbound

Comment 11

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/56e5195bef5b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I don't see how a JS exception could be caused by adding a single new assert in C++ that does not change any logic. If this caused failures of any kind, it would be assertion failure crashes. I think that is just an extant latent intermittent.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d9cbc1fdbe9

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/0d9cbc1fdbe9

Comment 16

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

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 17

[Steven DeTar [:sdetar]]

Jason, ideas on what to do with this bug?  Should we close it?

Comment 18

[Jason Orendorff [:jorendorff]]

Nick, is this done?

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

We landed release assertions that should catch this bug if normal control flow is happening, but AFAIK, they never did catch it. Therefore I think control flow is getting corrupted or the stacks are bad. Given that the crash signature is still seeing activity, it looks like this is still happening.

I'm not sure what the next steps are now, though.

Comment 20

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

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 21

[Steven DeTar [:sdetar]]

We will leave this open for a bit more time, but consider closing in 6 months if it has not been closed by then.

Comment 22

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

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 23

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

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 24

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

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 25

[Steven DeTar [:sdetar]]

Still leaving this open for now

Comment 26

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

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 27

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

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 28

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

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 29

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

Since the crash volume is low (less than 5 per week), the severity is downgraded to S3. Feel free to change it back if you think the bug is still critical.

For more information, please visit auto_nag documentation.

Comment 30

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

Closing because no crashes reported for 12 weeks.