Closed
Bug 943392
Opened 11 years ago
Closed 11 years ago
MemoryTextureClient should use a fallible allocator
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: nical, Assigned: nical)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
BenWa
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Allocating big buffers with the infallible allocator is not safe, especially with the memory constraints we have on some of the B2G devices.
Comment 1•11 years ago
|
||
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?
Assignee | ||
Comment 2•11 years ago
|
||
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 | ||
Comment 3•11 years ago
|
||
Assignee: nobody → nical.bugzilla
Attachment #8341746 -
Flags: review?(bgirard)
Comment 4•11 years ago
|
||
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-
Assignee | ||
Comment 5•11 years ago
|
||
Oops, you are right!
Attachment #8341746 -
Attachment is obsolete: true
Attachment #8342324 -
Flags: review?(bgirard)
Comment 6•11 years ago
|
||
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)
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
Comment on attachment 8342324 [details] [diff] [review]
Let MemoryTextureClient use fallible allocation
Alright lets land it
Attachment #8342324 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 9•11 years ago
|
||
Comment 10•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 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?
Updated•11 years ago
|
Attachment #8342324 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•11 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•