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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[alex_mayorga]

This bug was filed from the Socorro interface and is 
report bp-04ba7f23-ca2e-47ba-83f4-00a1c2141204.
=============================================================

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 2

[Boris Chiou [:boris]]

(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.

Comment 3

[Boris Chiou [:boris]]

(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 just forget to add a nullptr check in the ImageHost::Lock() and AutoLockCompositableHost to prevent any malicious usage.

Comment 4

[Boris Chiou [:boris]]

(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.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

That's probably worth a speculative fix and we can see if it shows in the crash reports?

Comment 6

[Boris Chiou [:boris]]

Attachment: Add nullptr check in ImageHost and CompositableHost (v1)

Comment 7

[Boris Chiou [:boris]]

(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 8

[Nicolas Silva [:nical]]

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.

Comment 9

[Boris Chiou [:boris]]

(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

Comment 10

[Boris Chiou [:boris]]

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.

Comment 11

[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.

Comment 12

[Aaron Train [:aaronmt]]

Land this?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6d29b59c3f11

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/6d29b59c3f11