Closed Bug 1108449 Opened 5 years ago Closed 5 years ago

crash in libxul.so@0x6ca03b | mozilla::layers::ImageHost::Lock()

Categories

(Firefox for Android :: General, defect, critical)

All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 38

People

(Reporter: alex_mayorga, Assigned: boris)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-04ba7f23-ca2e-47ba-83f4-00a1c2141204.
=============================================================
Boris, any thoughts on this one?  Seems to be happening in the code you added in bug 975346, but that was back in 33, and this crash seems to have shown up later.  It's still possibly related, and the bug could have stayed dormant.
Flags: needinfo?(boris.chiou)
Flags: needinfo?(boris.chiou)
(In reply to Milan Sreckovic [:milan] from comment #1)
> Boris, any thoughts on this one?  Seems to be happening in the code you
> added in bug 975346, but that was back in 33, and this crash seems to have
> shown up later.  It's still possibly related, and the bug could have stayed
> dormant.

mFrontBuffer should not be nullptr in the lock() function (because it has early return in ImageHost::Composite()) and the registers in the report didn't have any nullptr.

Actually, bug 975346 only used a new autolock in ImageHost and didn't change its logic. Maybe we should try to reproduce locally to handle this issue.
(In reply to Boris Chiou [:boris] from comment #2)
> (In reply to Milan Sreckovic [:milan] from comment #1)
> > Boris, any thoughts on this one?  Seems to be happening in the code you
> > added in bug 975346, but that was back in 33, and this crash seems to have
> > shown up later.  It's still possibly related, and the bug could have stayed
> > dormant.
> 
> mFrontBuffer should not be nullptr in the lock() function (because it has
> early return in ImageHost::Composite()) and the registers in the report
> didn't have any nullptr.
> 
> Actually, bug 975346 only used a new autolock in ImageHost and didn't change
> its logic. Maybe we should try to reproduce locally to handle this issue.

However, I think I forgot to add nullptr checks in the ImageHost::Lock() and AutoLockCompositableHost to prevent any malicious usage.
That's probably worth a speculative fix and we can see if it shows in the crash reports?
Assignee: nobody → boris.chiou
(In reply to Milan Sreckovic [:milan] from comment #5)
> That's probably worth a speculative fix and we can see if it shows in the
> crash reports?

OK. Let's try this patch. :)
Comment on attachment 8539768 [details] [diff] [review]
Add nullptr check in ImageHost and CompositableHost (v1)

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

Drive-by review, looks good.
Attachment #8539768 - Flags: review+
(In reply to Nicolas Silva [:nical] from comment #8)
> Comment on attachment 8539768 [details] [diff] [review]
> Add nullptr check in ImageHost and CompositableHost (v1)
> 
> Review of attachment 8539768 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive-by review, looks good.

Thanks, Nicolas.

Try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3728ac2c962
Hi, Alex, 
Do you know any possible situation to reproduce this crash? I wonder how to reproduce this crash, so QA and me can make sure what the root cause is and verify our patches on our machines. Thanks.
Flags: needinfo?(alex_mayorga)
¡Hola Boris!

I don't really have steps, this popped up at about the same time as the VisitAbove insta crash galore of recent past week or so, see https://bugzilla.mozilla.org/show_bug.cgi?id=1108451 and https://bugzilla.mozilla.org/show_bug.cgi?id=1111426 that happened at around that time.
Flags: needinfo?(alex_mayorga)
Land this?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6d29b59c3f11
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.