1152972 - crash in rx::IndexRangeCache::ComputeRange(unsigned int, void const*, int)

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[(Away)]

This bug was filed from the Socorro interface and is 
report bp-0f483195-ad7b-4096-a594-47b212150404.
=============================================================

#16 crash on 37.0.1 release. Always on Win7Sp1.

Frame 	Module 	Signature 	Source
0 	libglesv2.dll 	rx::IndexRangeCache::ComputeRange(unsigned int, void const*, int) 	gfx/angle/src/libglesv2/renderer/IndexRangeCache.cpp
1 	libglesv2.dll 	gl::ValidateDrawElements(gl::Context*, unsigned int, int, unsigned int, void const*, int, rx::Range<unsigned int>*) 	gfx/angle/src/libglesv2/validationES.cpp
2 	libglesv2.dll 	glDrawElements 	gfx/angle/src/libglesv2/libGLESv2.cpp

Comment 1

[(Away)]

I don't see this on pre-release channels >37, but it's possible this only shows up on release (since we didn't see it when 37 was pre-release).

Milan do you know of anything that may have fixed this? Otherwise let's assume it will persist when 38 rolls out.

Comment 2

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

I think we did some uplifts to 38 with ANGLE, but Jeff knows better.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Yes, there was an ANGLE update in 38

Comment 4

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

If we're not seeing this past 37.0.1, it was likely the ANGLE update, and I don't think we'd propose the uplift to 37 at this point.

Comment 5

[Robert Kaiser]

According to a search, this is the largest crash with 37.0.1 in the last week on Google Maps: https://crash-stats.mozilla.com/search/?product=Firefox&version=37.0.1&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 (you need to have permissions to see the URLs of crashes to make this search work correctly - if you do not, the url filters will not be displayed on that page nor searched for).

Are any of the following related or are they a different bug?
https://crash-stats.mozilla.com/report/list?signature=rx%3A%3ACopyNativeVertexData%3Cunsigned+short%2C+int%2C+0%3E%28unsigned+char+const%2A%2C+unsigned+int%2C+unsigned+int%2C+unsigned+char%2A%29
https://crash-stats.mozilla.com/report/list?signature=rx%3A%3ACopyNativeVertexData%3Cfloat%2C+int%2C+0%3E%28unsigned+char+const%2A%2C+unsigned+int%2C+unsigned+int%2C+unsigned+char%2A%29

Comment 6

[(Away)]

(In reply to Robert Kaiser (:[e-mail]) from comment #5)
> Are any of the following related or are they a different bug?
> https://crash-stats.mozilla.com/report/
> list?signature=rx%3A%3ACopyNativeVertexData%3Cunsigned+short%2C+int%2C+0%3E%2
> 8unsigned+char+const%2A%2C+unsigned+int%2C+unsigned+int%2C+unsigned+char%2A%2
> 9
> https://crash-stats.mozilla.com/report/
> list?signature=rx%3A%3ACopyNativeVertexData%3Cfloat%2C+int%2C+0%3E%28unsigned
> +char+const%2A%2C+unsigned+int%2C+unsigned+int%2C+unsigned+char%2A%29

It is suspicious to me that those too show up only on Win7SP1. I can't tell whether they're the same issue as this bug though (but those two definitely look related to each other).

By the way, I filed bug 1156300 about the broken links for these libGLESv2 files.

Comment 7

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

OK, yeah, this is a simple missing null pointer check in the previous version of ANGLE, fixed with the one we're using in 38.  It's a one liner, but specific to 37.  I'll attach it to this bug anyway, and we can have a conversation about whether it warrants going into 37.0.* or not.

Comment 8

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

The version of ANGLE that has gl::Error getData(const uint8_t **outData) function should be OK, and the version of ANGLE that has void* Buffer11::getData() has a problem.  But if I clone mozilla-release, I get the "good one".  Am I looking in the wrong place?

Comment 9

[(Away)]

Probably the 38.0.5 stuff from Sylvestre's mail. Try rewinding m-r a bit?

Comment 10

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

Attachment: Don't derefence null pointer. r=jgilberg

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Largest 37.0.1 Google Maps crash.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: None, this function allows early returns and we're just checking null pointers.
[String/UUID change made/needed]:

If we decide to do another 37.* release, this patch should stop the crash.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8596780 [details] [diff] [review]
Don't derefence null pointer. r=jgilberg

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

Isn't 38 release now?

Comment 12

[Robert Kaiser]

(In reply to Milan Sreckovic [:milan] from comment #8)
> But if I clone mozilla-release, I get the "good one".  Am I looking in the wrong place?

As dmajor says, that's probably because we already uplifted 38 to the mozilla-release repository. If you look at the FIREFOX_37_0_2_RELEASE tag, you'll find the version that we have on releases right now.

I think we want to avoid doing further 37.0.x releases, but if we do, we'll probably do that off the relbranch that this tag is on.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8596780 [details] [diff] [review]
Don't derefence null pointer. r=jgilberg

Let's do it!

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Should be it 38 beta 8

Comment 15

[(Away)]

Comment on attachment 8596780 [details] [diff] [review]
Don't derefence null pointer. r=jgilberg

Wait - 38 doesn't need this fix. I believe Milan was requesting m-r approval in case we do another 37.0.x

Comment 16

[Sylvestre Ledru [:Sylvestre]]

ok, sorry, my bad. AFAIK, we are not planning to do a new 37 .

Comment 17

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

Right, we don't have a driver right now; this was just going to be in line to get picked up in case we do have a new 37.

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

I take it that 38+ are not affected. I have marked 37 as wontfix and am resolving this bug. Please reopen if a fix is still required.