551392 - Spurious subprocess crashes can be detected

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

nsExceptionHandler keeps a map of minidumps gathered from subprocesses, but we never remove entries from the map.  That means that if the browser is unlucky and a later subprocess is assigned the same PID/HANDLE as an earlier one that crashed, we might report that later one crashed as well.

I think a better API than CrashReporter::GetMinidumpForChild() is something like CrashReporter::TakeMinidumpForChild() that removes the child's dump from the map if it exists.

Ted, does that suit you?

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Remove subprocess minidumps after they're grabbed to avoid spurious crash detection

Comment 2

[(not currently active) Ted Mielczarek]

Sounds good, yes. (Will review later.)

Comment 3

[Benjamin Smedberg]

Lower priority, doesn't block beta.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Remove hack made unnecessary by unified build tiers

Sort of off-topic for this bug, but I'd like to get this in anyway.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Remove hack made unnecessary by unified build tiers, v2

Missed a spot

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Remove hack made unnecessary by unified build tiers, v3

Needed some fixups on windows because of HANDLE.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 432284 [details] [diff] [review]
Remove hack made unnecessary by unified build tiers, v3

This is good, but you're going to need to #ifdef MOZ_CRASHREPORTER around the #include and the call in ActorDestroy.

r=me with that fixed.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/dd36815ae7bb
http://hg.mozilla.org/mozilla-central/rev/bda8efa53512

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I suspect the second patch may have caused http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268960226.1268962345.4351.gz .

Comment 10

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/7b4edfb90d75

Not sure what's up with this bug. The first patch stuck, right? Can we mark this FIXED for branch-tracking purposes and file a followup if necessary for whatever remains?

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yeah, the important part stuck.  "Followup" (independent actually) is bug 554682.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 13

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430