Closed Bug 1344074 Opened 3 years ago Closed 3 years ago

Crash in IPC::ReadParam<T> with IPC::ReadParam<mozilla::Array<mozilla::gfx::Color, 4> >

Categories

(Core :: IPC, defect, critical)

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: mccr8, Assigned: kanru)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-181cd5b0-796e-4093-a771-5090c2170301.
=============================================================

I've seen this crash a few times when triaging Nightly crashes, so I'd figure I'd get a bug on file. We are in a stack overflow, with IPC::ReadParam<mozilla::Array<mozilla::gfx::Color, 4> > apparently recursively calling itself over and over again. It doesn't make much sense. There have only been 16 crashes in the last week, and they are all from a single install, so maybe somebody's machine is doing something weird? But why this crash, across multiple weeks?

This is rare enough that it probably isn't really worth investigating, but if Kanru or somebody gets curious maybe a minidump would provide more useful information. The actual code for deserializing mozilla::Array looks okay to me.
(In reply to Andrew McCreight [:mccr8] from comment #0)
> This bug was filed from the Socorro interface and is 
> report bp-181cd5b0-796e-4093-a771-5090c2170301.
> =============================================================
> 
> I've seen this crash a few times when triaging Nightly crashes, so I'd
> figure I'd get a bug on file. We are in a stack overflow, with
> IPC::ReadParam<mozilla::Array<mozilla::gfx::Color, 4> > apparently
> recursively calling itself over and over again. It doesn't make much sense.
> There have only been 16 crashes in the last week, and they are all from a
> single install, so maybe somebody's machine is doing something weird? But
> why this crash, across multiple weeks?
> 
> This is rare enough that it probably isn't really worth investigating, but
> if Kanru or somebody gets curious maybe a minidump would provide more useful
> information. The actual code for deserializing mozilla::Array looks okay to
> me.

The ParamTraits for mozilla::Array is actually not correct. It tries to pass &aResult[i] to ReadParam; aResult is a paramType* so &aResult[i] has type paramType* and we get a infinite loop.
Ah, good catch! That line looked a little funny to me but somehow I convinced myself it was okay, or that if it was that broken we'd see it more.
Comment on attachment 8843267 [details]
Bug 1344074 - make sure ParamTraits<mozilla::Array<T, Length>>::Read use the correct type.

https://reviewboard.mozilla.org/r/117074/#review118868
Attachment #8843267 - Flags: review?(matt.woodrow) → review+
Pushed by kchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ebbc4be47d0a
make sure ParamTraits<mozilla::Array<T, Length>>::Read use the correct type. r=mattwoodrow
Comment on attachment 8843267 [details]
Bug 1344074 - make sure ParamTraits<mozilla::Array<T, Length>>::Read use the correct type.

Approval Request Comment for 53
[Feature/Bug causing the regression]: bug 1319626
[User impact if declined]: In some case it will trigger a infinite recursion and crash by stack overflow in the end.
[Is this code covered by automated tests?]: I'm not sure. If it was covered it should be caught somehow.
[Has the fix been verified in Nightly?]: Manually tested.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The change can be statically verified by the compiler.
[String changes made/needed]: No.
Attachment #8843267 - Flags: approval-mozilla-aurora?
Comment on attachment 8843267 [details]
Bug 1344074 - make sure ParamTraits<mozilla::Array<T, Length>>::Read use the correct type.

Regression from 53, minor crash fix, let's take it before 53 moves to beta.
Attachment #8843267 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee: nobody → kchen
https://hg.mozilla.org/mozilla-central/rev/ebbc4be47d0a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.