1151736 - Crash @ mozilla::WebGLTexture::EnsureNoUninitializedImageData(StrongGLenum<TexImageTargetDetails>, int)

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

This is a follow up from bug 1090518. Socorro [1] indicates this crash is still showing, and also people kept encountering it after the initial fix while using www.ro.me and google maps (see final comments in bug 1090518).

Some crash reports:
bp-72dfd7db-581b-41b3-80b8-4ba732150401 - google maps
bp-b0d04f99-c769-496e-9142-36cdd2150404 - google maps


[1] - https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3AWebGLTexture%3A%3AEnsureNoUninitializedImageData%28StrongGLenum%3CTexImageTargetDetails%3E%2C+int%29#tab-sigsummary

Comment 1

[Robert Kaiser]

Looking in the signature summary (linked from the Crash Signature field), this affects all current versions. Also, it is the #1 bug on Google maps on 38 beta from all I can tell, with 30% of all crashes in that version on that site: https://crash-stats.mozilla.com/search/?product=Firefox&version=38.0&release_channel=beta&process_type=browser&process_type=content&url=~google&url=~%2Fmaps%2F&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature (that search only works when you have privs to see URLs on Socorro)

Comment 2

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

Are these all MOZ_CRASH crashes?  Dan, Jeff, is this really unrecoverable error?  Should we only crash if it is in fact OOM scenario?

Comment 3

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

Attachment: Get the GL error into crash reports. r=jgilbert

Comment 4

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

Let's land the patch with extra info, just to see what type of GL error we're getting.  Does not actually fix the bug, so this should stay open.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c730fbf7e12e

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

[Kelsey Gilbert [:jgilbert]]

(In reply to Milan Sreckovic [:milan] from comment #2)
> Are these all MOZ_CRASH crashes?  Dan, Jeff, is this really unrecoverable
> error?  Should we only crash if it is in fact OOM scenario?

We should really either expose the GL_OOM or force-lose the context, rather than crashing.

Comment 8

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

Sounds like a plan; Dan, can you get to this early next week?

Comment 9

[u480271]

I can tackle this. Which option do we want?
1) Expose OOM,
2) force-lose the context.

Comment 10

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

Good question; maybe we start with #2 if the error we get back is *not* GL_OUT_OF_MEMORY?

Comment 11

[Kelsey Gilbert [:jgilbert]]

We'd need a more detailed look at the code to do anything other than #2, to make sure we've left things in at least a reasonably safe state.

Comment 12

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

I can find no crashes that are *not* 0x505, which is GL_OUT_OF_MEMORY.

How do we expose this as OOM?

Comment 13

[Kelsey Gilbert [:jgilbert]]

(In reply to Milan Sreckovic [:milan] from comment #12)
> I can find no crashes that are *not* 0x505, which is GL_OUT_OF_MEMORY.
> 
> How do we expose this as OOM?

Force-lose the context. The issue with OOM is that it doesn't guarantee that the state of the context is sane after it is generated.

Comment 14

[u480271]

The MOZ_CRASH should be replaced by forced context loss when texImage upload returns OOM. See patch attached to Bug 1174151 for example of how to do this.

Comment 15

[Kyle Fung]

Attachment: lose-context-on-oom.patch

Checks if an OOM error was returned and loses the context if it is. If any other error is returned, it calls MOZ_CRASH().

Comment 16

[Kyle Fung]

Attachment: lose-context-on-oom-v2.patch

Added in changes from patch in Bug 1174151 along with suggested changes.

Comment 17

[u480271]

Comment on attachment 8623848 [details] [diff] [review]
lose-context-on-oom-v2.patch

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

::: dom/canvas/WebGLTexture.cpp
@@ +652,5 @@
>                                                // earlier.
>  
> +    size_t byteCount = checked_byteLength.value();
> +
> +    UniquePtr<uint8_t> zeros((uint8_t*)calloc(1, byteCount));

OK. moz_calloc() doesn't exist.

@@ +699,5 @@
>          // Should only be OUT_OF_MEMORY. Anyway, there's no good way to recover
>          // from this here.
>          gfxCriticalError() << "GL context GetAndFlushUnderlyingGLErrors " << gfx::hexa(error);
>          printf_stderr("Error: 0x%4x\n", error);
> +        if (error == LOCAL_GL_OUT_OF_MEMORY) {

I feel it's better to format this way:

if (error != LOCAL_GL_OUT_OF_MEMORY) {
    // Errors on texture upload have been related to video
    // memory exposure in the past, which is a security issue.
    // Crash on purpose.
    MOZ_CRASH();
}

// Out-of-memory uploading pixels to GL. Lose context and report OOM.
mContext->ForceLoseContext(true);
mContext->ErrorOutOfMemory("EnsureNoUninitializedImageData: Failed to "
                                 "upload texture of width: %u, height: %u, "
                                 "depth: %u to target %s level %d.",
                                 imageInfo.mWidth, imageInfo.mHeight, imageInfo.mDepth,
                                 mContext->EnumName(imageTarget.get()), level);
return false;

Comment 18

[u480271]

Comment on attachment 8623848 [details] [diff] [review]
lose-context-on-oom-v2.patch

Since I wrote most of the patch, I'm going to CC Jeff on this review so I'm not OK'ing my own code.

Comment 19

[Kyle Fung]

Attachment: lose-context-on-oom-v3.patch

Changed formatting of error handling code.

Comment 20

[Kyle Fung]

Attachment: lose-context-on-oom-v3.patch

Changed the reviewer name in commit message.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8624019 [details] [diff] [review]
lose-context-on-oom-v3.patch

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

Sorry for the delay!

::: dom/canvas/WebGLTexture.cpp
@@ +703,5 @@
> +        if (error != LOCAL_GL_OUT_OF_MEMORY) {
> +            // Errors on texture upload have been related to video
> +            // memory exposure in the past, which is a security issue.
> +            // Crash on purpose.
> +            MOZ_CRASH();

We should be fine forcing context loss instead of crashing.

Comment 22

[Kyle Fung]

Attachment: lose-context-on-oom-v4.patch

If an error other than OOM is given, then lose context rather than crash.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45225813dd04

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f318a1489c4

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f318a1489c4

Comment 25

[alex_mayorga]

Report ID 	Date Submitted
bp-ddbd2d65-79b9-40dc-b4a4-12d432150825
	25/08/2015	12:47 p.m.

Comment 26

[alex_mayorga]

¡Hola Kyle!

Any chance to land this on Beta?

Report ID 	Date Submitted
bp-39d408ff-161f-40bf-95e6-d3f882150904
	03/09/2015	09:13 p.m.

¡Gracias!

Comment 27

[Kyle Fung]

Comment on attachment 8627689 [details] [diff] [review]
lose-context-on-oom-v4.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1151736
[User impact if declined]: Crashes in Google Maps
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: No risks that I can see.
[String/UUID change made/needed]:

Comment 28

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

Comment on attachment 8627689 [details] [diff] [review]
lose-context-on-oom-v4.patch

I checked crash-stats and we are hitting this crash in every 41.0 Beta so far though at a low volume. Given that this patch has been in Nightly, Aurora for over two months, it seems safe to uplift to Beta41.