crash in rx::Buffer11::updateBufferStorage since Firefox 44

RESOLVED FIXED

Status

()

Core
Graphics
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: philipp, Assigned: mtseng)

Tracking

({crash, regression})

44 Branch
x86
Windows NT
crash, regression
Points:
---

Firefox Tracking Flags

(firefox44 wontfix, firefox45 wontfix, firefox46 wontfix, firefox47+ wontfix, firefox48+ wontfix, firefox49+ wontfix, firefox-esr45 affected)

Details

(Whiteboard: [gfx-noted], crash signature)

Attachments

(1 obsolete attachment)

(Reporter)

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-fb812a70-e18e-4280-9f7d-4c7442160213.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	libglesv2.dll 	rx::Buffer11::updateBufferStorage(rx::Buffer11::BufferStorage*, unsigned int, unsigned int) 	gfx/angle/src/libangle/renderer/d3d/d3d11/Buffer11.cpp
1 	libglesv2.dll 	rx::Buffer11::getBufferStorage(rx::BufferUsage) 	gfx/angle/src/libangle/renderer/d3d/d3d11/Buffer11.cpp
2 	libglesv2.dll 	rx::Buffer11::getBuffer(rx::BufferUsage) 	gfx/angle/src/libangle/renderer/d3d/d3d11/Buffer11.cpp
3 	libglesv2.dll 	rx::InputLayoutCache::applyVertexBuffers(std::vector<rx::TranslatedAttribute, std::allocator<rx::TranslatedAttribute> > const&, unsigned int, gl::Program*, rx::SourceIndexData*) 	gfx/angle/src/libangle/renderer/d3d/d3d11/InputLayoutCache.cpp
4 	libglesv2.dll 	rx::Renderer11::applyVertexBuffer(gl::State const&, unsigned int, int, int, int, rx::SourceIndexData*) 	gfx/angle/src/libangle/renderer/d3d/d3d11/Renderer11.cpp

this is a new crash signature on windows 7 & above since firefox 44 and happening across gpu vendors. multiple crash comments seem to link this to visiting google maps.
(Reporter)

Updated

2 years ago
Component: General → Graphics

Updated

2 years ago
Flags: needinfo?(milan)
Most likely out of memory, ANGLE not ready for it in this case.  Not sure why it just started showing up in 44.
Flags: needinfo?(milan)
Whiteboard: [gfx-noted]
Created attachment 8733896 [details]
MozReview Request: Bug 1248276: getStagingStorage could return null. Wallpaper, speculative fix. r?jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41993/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41993/
Attachment #8733896 - Flags: review?(jmuizelaar)
Assignee: nobody → milan
Have you tested this? i.e. what happens if you force getStagingStorage() to return null?
Flags: needinfo?(milan)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> Have you tested this? i.e. what happens if you force getStagingStorage() to
> return null?

I did not test it.  That would be a good test to run :)  I did observe that we call getStagingStorage() elsewhere in Buffer11 methods and the other places check the return for nullptr and return a GL_OUT_OF_MEMORY.  Unfortunately, this particular method doesn't have a return value, so its callers may very well not be ready for it.
Comment on attachment 8733896 [details]
MozReview Request: Bug 1248276: getStagingStorage could return null. Wallpaper, speculative fix. r?jrmuizel

We're not ready for this failure further in the code.  Perhaps the new ANGLE takes care of this, though it's a major change, so even if it does, it isn't necessarily something we could uplift.
Flags: needinfo?(milan)
Attachment #8733896 - Flags: review?(jmuizelaar)
Sounds like you are still investigating and this is not a topcrash on beta. We could still take a patch for 47 but it's too late for 46.
status-firefox45: affected → wontfix
status-firefox46: affected → wontfix
status-firefox48: --- → affected
tracking-firefox47: --- → +
tracking-firefox48: --- → +
Comment on attachment 8733896 [details]
MozReview Request: Bug 1248276: getStagingStorage could return null. Wallpaper, speculative fix. r?jrmuizel

When I force this situation, bad things happen further down.  See comment 5.
Attachment #8733896 - Attachment is obsolete: true
Peter, is there somebody that can look at this on your side?
Assignee: milan → nobody
Flags: needinfo?(howareyou322)

Comment 9

2 years ago
Morris, please help to check any problem if getStagingStorage() returns null.
Assignee: nobody → mtseng
Flags: needinfo?(howareyou322)
Hi Milan, is this something we might have a WIP fix for and want to uplift to Fx47? Or is this a wontfix for Fx47? I think this one doesn't have as much volume on Aurora/Beta channels as it does not release.
Flags: needinfo?(milan)
Don't see this getting done in 47 if it turns out to be an ANGLE problem, or an out of memory problem.

Morris, have you had a chance to look at this?
Flags: needinfo?(milan) → needinfo?(mtseng)
Thanks Milan. This is now a wontfix for Fx47, if a patch is ready next week and deemed low risk, I would be willing to reconsider for Fx47.
status-firefox47: affected → wontfix
I'll check this.
Flags: needinfo?(mtseng)
Looks like latest version of ANGLE handle this. See https://github.com/google/angle/blob/master/src/libANGLE/renderer/d3d/d3d11/Buffer11.cpp#L770

Should we update ANGLE to latest version?
Flags: needinfo?(jmuizelaar)
I just queried the number of this crash.

Windows 7 	647 	76.7%
Windows 10 	166 	19.7%
Windows 8.1 	27 	3.2%
Windows 8 	3 	0.4%

Milan, how do you think the next step for this bug? Maybe we just address this issue for next ANGLE update.
Flags: needinfo?(jmuizelaar) → needinfo?(milan)
(In reply to Peter Chang[:pchang] from comment #15)
> I just queried the number of this crash.
> 
> Windows 7 	647 	76.7%
> Windows 10 	166 	19.7%
> Windows 8.1 	27 	3.2%
> Windows 8 	3 	0.4%
> 
> Milan, how do you think the next step for this bug? Maybe we just address
> this issue for next ANGLE update.
s/for/when
Let's aim for the next ANGLE update, but if there is a limited change that we can cherry pick out of the upstream version, so that we can uplift to 48, we won't have to wait until (most likely) version 50 before this gets fixed.
100 crashes a day is reasonable high that it's worth doing it.
Flags: needinfo?(milan)
(In reply to Milan Sreckovic [:milan] from comment #17)
> Let's aim for the next ANGLE update, but if there is a limited change that
> we can cherry pick out of the upstream version, so that we can uplift to 48,
> we won't have to wait until (most likely) version 50 before this gets fixed.
> 100 crashes a day is reasonable high that it's worth doing it.
Agree.

Morris, could you identify the change scope to fix this issue?
Flags: needinfo?(mtseng)
I'll cherry-pick changeset from ANGLE.
Flags: needinfo?(mtseng)
The changeset [1] is not easy to merge into our current ANGLE because the patch is huge and have many dependencies with other changeset. Should we aim for next ANGLE update instead?


[1]: https://github.com/google/angle/commit/da1233080f27b0c855a1ffc8f635aebc4d9c6af3
Flags: needinfo?(milan)
If there isn't a small patch we can pick out, yes, we should just go for the next ANGLE update.
Flags: needinfo?(milan)

Updated

2 years ago
status-firefox44: affected → wontfix
According to comment #21, we are going to wait for update for ANGLE. We don't want to take ANGLE update in beta 48 and we will see 49 and 50.
status-firefox48: affected → wontfix
status-firefox49: --- → affected
tracking-firefox49: --- → +
Depends on: 1281687
Crash volume for signature 'rx::Buffer11::updateBufferStorage':
 - nightly (version 50): 0 crash from 2016-06-06.
 - aurora  (version 49): 40 crashes from 2016-06-07.
 - beta    (version 48): 513 crashes from 2016-06-06.
 - release (version 47): 3792 crashes from 2016-05-31.
 - esr     (version 45): 1232 crashes from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          0          0          0          0          0          0          0
 - aurora           3          4          3          4         16          8          0
 - beta            84         62         82         90         65         76         19
 - release        621        486        587        611        573        486        152
 - esr            142        129        152        136        130        134        108

Affected platform: Windows
status-firefox-esr45: --- → affected
Fixed by ANGLE update.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Hi Morris,
Is there anything we need to do here given the ANGLE is updated?
Flags: needinfo?(mtseng)
Nope. ANGLE update fix this issue. We don't need do anything here.
Flags: needinfo?(mtseng)
After discussing with Morris, the patch of dependent bug 1281687 is pretty big and it's very risky to uplift this to 49. Let's let it ride the train on 50. Mark 49 as won't fix.
status-firefox49: affected → wontfix
See Also: → bug 1325511
You need to log in before you can comment on or make changes to this bug.