1308125 - Crash in js::gc::MakePagesReadOnly

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect)

Description

[Tabs16 (:Tabs16)]

This bug was filed from the Socorro interface and is 
report bp-b79b9b98-9f10-4ca2-9cb1-1677c2161005.
=============================================================
It started with Build ID 20161004030204 on 2016-10-05 13:03:14 and it is still continuing, latest crash on Build ID 20161005030211  .
The crash is on Firefox 52.0a1 version,Firefox 50 version and Firefox 49 version,Windows NT
Maximum crashes encountered on October 3,2016.
Uptime range: <1 min   
The link to recent crashes http://bit.ly/2duBANu

Comment 1

[Tabs16 (:Tabs16)]

Correction: Maximum crashes occured on 5th October at 00:00.

Comment 2

[Tabs16 (:Tabs16)]

Hi Emanuel,
Since you were the last one to touch code in this area, perhaps you can shed some light on the crash?

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

Thanks for the heads up! Hmm, I'm not sure what could be going on here - nothing should have changed on the build from the 4th in particular, but more importantly I'm not sure why VirtualProtect would fail. The new buffer shouldn't be null - even on OOM we just return the old buffer, which we should still be able to protect.

I wonder if this somehow explains our missing crashes, even if it's only on Windows. I can try adding some more assertions here and add the last error code to the MOZ_CRASH reason.

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Diagnose MakePagesReadOnly crashes and fix crash reporter annotations.

Ugh, I don't think this explains the missing crashes, but I think I just figured it out. Turns out I misread how MOZ_CRASH_ANNOTATE() is used: the macros using it add a MOZ_CRASH() or a MOZ_RELEASE_ASSERT() around the string literal passed in. This patch fixes that, so hopefully socorro will be able to parse them now.

For this bug, I added two release assertions and added the error code to the crash reason. You can't pass a dynamic string to MOZ_CRASH(), so that was a bit finicky (which is how I figured out my mistake), but this should work. I added the same code to ProtectPages() and UnprotectPages() for good measure.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8798402 [details] [diff] [review]
Diagnose MakePagesReadOnly crashes and fix crash reporter annotations.

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

::: js/src/gc/Memory.cpp
@@ +853,4 @@
>  #if defined(XP_WIN)
>      DWORD oldProtect;
> +    if (!VirtualProtect(p, size, PAGE_NOACCESS, &oldProtect)) {
> +        snprintf(sCrashReason, 256,

Nit: s/256/sizeof(sCrashReason)/, and below ?

Comment 6

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: v1.1 - Diagnose MakePagesReadOnly crashes and fix crash reporter annotations.

Thanks! Carrying forward r+.

From discussion on IRC, this probably won't change anything for Socorro after all, but it's probably worth being explicit anyway (in addition to the diagnostics for this bug).

(In reply to Jan de Mooij [:jandem] from comment #5)
> > +        snprintf(sCrashReason, 256,
> 
> Nit: s/256/sizeof(sCrashReason)/, and below ?

Good point, done. I also changed from %x to %u, since it was either that or add 0x in front, and the error codes are listed as decimal numbers anyway [1].

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms681381.aspx

Comment 7

[Emanuel Hoogeveen [:ehoogeveen]]

Try run looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39fbd07324ff3fa313b01288d90b97a1ec125e21

Pretty much build-only, but if we were seeing crashes from this in automation they would have showed up already.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d0a761ff566
Diagnose MakePagesReadOnly crashes and fix crash reporter annotations. r=jandem

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d0a761ff566

Comment 10

[Emanuel Hoogeveen [:ehoogeveen]]

I set this as blocking the wrong bug, and the instrumentation I added won't be useful until bug 1309573 is fixed (we also haven't seen any more of these crashes though).

Comment 11

[Emanuel Hoogeveen [:ehoogeveen]]

We've gotten a few crashes in UnprotectPages that say the address is invalid, which may be due to bitflips. No more crashes in MakePagesReadOnly though, and we don't use it anymore right now anyway.

Comment 12

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.