1198663 - crash in mozilla::layers::ImageContainer::GetCurrentSize

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Catalin Suciu [:csuciu]]

This bug was filed from the Socorro interface and is 
report bp-49ebe05a-bc15-4864-b169-4e9622150826.
=============================================================

Nightly/Aurora crash when trying to play a video on imdb.com on 
Samsung Galaxy Tab 2 7" (4.2.2)

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e7e69cc8c07b&tochange=9b902b7669ae

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: significant volume change in this signature for 42

Don't see much in the devices or graphics section that points to a single class of devices. Good distribution of devices, Android API versions and graphics hardware.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 43+. 
Margaret, can you help find an owner for this bug? I'll also ask on the crashdebug list.

Comment 3

[:Margaret Leibovic]

This is more in snorp's realm than mine.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

We didn't make any changes here, seems like a gfx regression?

Comment 5

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

This would suggest that we can have non-empty array in mCurrentImages, where the image in the first element is null.
How did we get the regression range?

Do we have an STR, and if so, can we run this in the debug build?  If mCurrentImages[0].mImage is actually null, we probably would have hit an assertion earlier on (ImageContainer::SetCurrentImageInternal ?)

Anyway, based on bug 1143575, in the regression range, Rob, any further thoughts?

There are also some sound library changes in the regression range (not sure if they affect Android) and some of the user comments talk about "sound is messed up", but we probably want to first rule out bug 1143575.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

#3 crasher on release (fx42)

Comment 7

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

Can anybody tell why this went up in crash volume?  Is it the devices, or the number of people we have in pre-release versions, or what those users do, or something else?  Just trying to figure out why we weren't seeing it as a top crasher until 42 went out, and if we can catch things like this in the future.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Milan Sreckovic [:milan] from comment #5)
> This would suggest that we can have non-empty array in mCurrentImages, where
> the image in the first element is null.

That sounds right.

> Do we have an STR, and if so, can we run this in the debug build?

No STR, as far as I can tell.

> If mCurrentImages[0].mImage is actually null, we probably would have hit an
> assertion earlier on (ImageContainer::SetCurrentImageInternal ?)

Only in debug builds.

> Anyway, based on bug 1143575, in the regression range, Rob, any further
> thoughts?

Almost certainly my bug.

Sprinkling some release asserts around should help us track this down.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1198663. Tolerate null Image in Android NPAPI plugins. r=snorp

Bug 1198663. Skip null Images in VideoSink::RenderVideoFrames instead of treating them as valid. r=jwwang

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I tried to check all the paths that could lead to a null image in NonOwningImage and this is the only one I could find, and it's clearly a bug.

Comment 11

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

Mark, how far would you want this uplifted?  Would you do a 42.0.1 for it?

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Milan Sreckovic [:milan] from comment #11)
> Mark, how far would you want this uplifted?  Would you do a 42.0.1 for it?

Once we verify, yes, I would do a 42.0.1 for it. Especially if we can get bug 1209612 too.

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8686244 [details]
MozReview Request: Bug 1198663. Tolerate null Image in Android NPAPI plugins. r=snorp

https://reviewboard.mozilla.org/r/24961/#review22523

Comment 14

[Kevin Brosnan [Ex-Mozilla]]

This build has Roc's patch. Ioana would someone from your team try to reproduce comment 0 with it? http://people.mozilla.org/~kbrosnan/tmp/1198663/fennec-45.0a1.en-US.android-arm.apk

Comment 15

[Catalin Suciu [:csuciu]]

(In reply to Kevin Brosnan [:kbrosnan] from comment #14)
> This build has Roc's patch. Ioana would someone from your team try to
> reproduce comment 0 with it?
> http://people.mozilla.org/~kbrosnan/tmp/1198663/fennec-45.0a1.en-US.android-
> arm.apk

This build still crashes when playing videos on imdb.com, but I'm getting a different signature:
https://crash-stats.mozilla.com/report/index/59baad1b-11df-41e7-a1c0-0881b2151112

Comment 16

[Kevin Brosnan [Ex-Mozilla]]

The signature generation will not work for personal builds.

Comment 17

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

:snorp, do you have a build handy to try running with the patch and see where we crash?  Trying to save a few hours before :roc is back online.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc2c76086aefc8f03c0cbec1eeb9d290415fec72
Bug 1198663. Skip null Images in VideoSink::RenderVideoFrames instead of treating them as valid. r=jwwang

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/bc2c76086aef

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can someone confirm that this bug is actually fixed?

Comment 21

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Can someone confirm that this bug is actually fixed?

And once we do, let's ask for the uplifts all the way.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

We probably want to track this for 42 as it may be included for a 42.0.1 for mobile.

Comment 23

[Catalin Suciu [:csuciu]]

Using the steps from comment #0, I'm still able to reproduce the crash on latest Nightly (2015-11-17)

https://crash-stats.mozilla.com/report/index/3674e313-bfdb-4036-b77b-1bf8d2151118

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Yes, I was aware of this bug. Thanks

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Reopening (comment #23)

Comment 26

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

Can somebody run a debug build?

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, debug builds have many asserts that would narrow this down a lot.

Comment 28

[Catalin Suciu [:csuciu]]

Attachment: log.txt

Logs from http://ftp.mozilla.org/pub/mobile/tinderbox-builds/mozilla-inbound-android-api-11-debug/1447912538/

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8686244 [details]
MozReview Request: Bug 1198663. Tolerate null Image in Android NPAPI plugins. r=snorp

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24961/diff/1-2/

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsPluginInstanceOwner::GetImageContainer asserted at "MOZ_ASSERT(img);". So this must be Flash-related.

It seems we'll hit this assert whenever nsNPAPIPluginInstance::mContentTexture and mContentSurface are both null.

Note that we really shouldn't be creating a new ImageContainer every frame. Oh well.

Comment 31

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8686244 [details]
MozReview Request: Bug 1198663. Tolerate null Image in Android NPAPI plugins. r=snorp

https://reviewboard.mozilla.org/r/24961/#review23373

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> 
> Note that we really shouldn't be creating a new ImageContainer every frame.
> Oh well.

At the time this was written I think there was a reason for that, but I can't remember now. I filed bug 1229378 to follow up.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d34e6e7a22d1aa638bfb902193f78cd446ce3f1e
Bug 1198663. Tolerate null Image in Android NPAPI plugins. r=snorp

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d34e6e7a22d1

Comment 35

[Catalin Suciu [:csuciu]]

Using the steps/device from comment #0, I cannot reproduce the crash on latest Nightly (2015-12-03)

Verifying as fixed

Comment 36

[Mark Finkle (:mfinkle) (use needinfo?)]

When we consider uplifting, remember that two patches landed:
https://hg.mozilla.org/mozilla-central/rev/bc2c76086aef
https://hg.mozilla.org/mozilla-central/rev/d34e6e7a22d1

The last one fixed the crash, but the first one was seen as a real bug too (comment 10).

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Roc, should we uplift the fix to Aurora44?

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, we should get this up to Aurora. Beta too, if it's not too late.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8686244 [details]
MozReview Request: Bug 1198663. Tolerate null Image in Android NPAPI plugins. r=snorp

Approval Request Comment
[Feature/regressing bug #]: 1143575
[User impact if declined]: Crashes
[Describe test coverage new/current, TreeHerder]: Lots of tests test this code
[Risks and why]: low risk. We're just avoiding null derefs.
[String/UUID change made/needed]: none

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8686244 [details]
MozReview Request: Bug 1198663. Tolerate null Image in Android NPAPI plugins. r=snorp

Please uplift both patches in comment 10 to aurora and beta.

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3b0724aa3186
https://hg.mozilla.org/releases/mozilla-beta/rev/573522753004

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-release/rev/7062715ca2d8
https://hg.mozilla.org/releases/mozilla-release/rev/dc6a24dab477

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ee34a0965c04
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e575218cea33

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e575218cea33
https://hg.mozilla.org/releases/mozilla-aurora/rev/ee34a0965c04

Comment 45

[u421692]

Using the steps/device from comment #0, I can't reproduce this on builds marked affected, so I can't mark this fixed on Firefox 43 Beta 10.
Will need to check the crash-stats after Firefox 43 Beta 10 will be on Play Store.

Comment 46

[Sylvestre Ledru [:Sylvestre]]

42 is gone.