MemoryTextureClient should use a fallible allocator

RESOLVED FIXED in Firefox 27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nical, Assigned: nical)

Tracking

unspecified
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27 fixed, firefox28 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

Allocating big buffers with the infallible allocator is not safe, especially with the memory constraints we have on some of the B2G devices.
Why is it not safe? infallible means we should crash which wont be exploitable. If we switch to fallible allocations we need a way to fallback gracefully. Do you have a suggestion how to fallback gracefully?
I think that an incorrectly rendered layer (I mean aborting the update of the layer if we could not allocate it, miss a few frames, whatever) with warnings in the logs is better than a crash.
It's certainly a debatable question.
Crashes are good to raise awarness of issues and for debuggability (and I'd gladly crash debug builds in this case) but there is nothing worse for users (with the exception of security vulnerabilities).
Assignee: nobody → nical.bugzilla
Attachment #8341746 - Flags: review?(bgirard)
Blocks: 900244
Comment on attachment 8341746 [details] [diff] [review]
Let MemoryTextureClient use fallible allocation.

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

The (indirect) result of Allocate isn't checked properly:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/SharedRGBImage.cpp#87
Attachment #8341746 - Flags: review?(bgirard) → review-
Oops, you are right!
Attachment #8341746 - Attachment is obsolete: true
Attachment #8342324 - Flags: review?(bgirard)
We keep having this discussion about what to do with layers when we can't allocate. I think we should take an official stance on this rather then be inconsistent.

We could also use a checkerboard like texture to indicate a layer couldn't be allocated.
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(jmuizelaar)
That seems fine to me. I don't think we need to both with checkerboarding though. Once we run so low on memory that we can't allocate screen sized surfaces the whole browser is about to fall apart anyway.

In the case where the texture client is considerably bigger than the screen, then we should look at not doing that.
Flags: needinfo?(matt.woodrow)
Comment on attachment 8342324 [details] [diff] [review]
Let MemoryTextureClient use fallible allocation

Alright lets land it
Attachment #8342324 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/mozilla-central/rev/0cfd889611f8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment on attachment 8342324 [details] [diff] [review]
Let MemoryTextureClient use fallible allocation

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: Crashes on OOM on some Android devices. See bug 945821.
Testing completed (on m-c, etc.): Haven't seen the same crash in Aurora+
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None
Attachment #8342324 - Flags: approval-mozilla-beta?
Attachment #8342324 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(jmuizelaar)
Whiteboard: [qa-]
Duplicate of this bug: 945821
You need to log in before you can comment on or make changes to this bug.