Last Comment Bug 774755 - Update ANGLE to r1242
: Update ANGLE to r1242
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on: 795117 798919
Blocks: gecko-games
  Show dependency treegraph
 
Reported: 2012-07-17 11:05 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-10-07 05:31 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
update the makefiles (12.36 KB, patch)
2012-07-18 15:15 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review
rename the preprocessor's Diagnostics.cpp file (357 bytes, patch)
2012-07-18 15:18 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review
rename the preprocessor's DirectiveHandler.cpp file (377 bytes, patch)
2012-07-18 15:18 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review
flip the WebGL canvas when using share handles in d3d10 layers (1.06 KB, patch)
2012-07-18 15:20 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review
fix ANGLE crash on null programBinary (4.24 KB, patch)
2012-07-18 15:21 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2012-07-17 11:05:15 PDT
This will give us:
 - the new preprocessor. The old one had been our #1 source of security bugs in ANGLE.
 - anisotropic texture filtering on Windows -> blocking gecko-games

Doing a full reset of our ANGLE copy (instead of just applying a diff) as the changes are very heavy and our local .patch files were not in a very good shape.

9%:
https://tbpl.mozilla.org/?tree=Try&rev=65e28130452e
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2012-07-17 11:10:02 PDT
Oh yes, I also wiped most of the CPPSRCS from libEGL/Makefile.in, so we should have a significantly smaller libEGL on Windows now.
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2012-07-17 11:54:52 PDT
Also, I'm stopping defining ANGLE_USE_NSPR because there is no need for that, instead using the windows or posix variants like Chromium does, as discussed with vlad.
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-07-17 13:53:40 PDT
Alright, libEGL shrinks from 84k to 58k... zip archive size shrinks by 16k.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2012-07-17 21:47:33 PDT
The Win7 oranges are expected-to-fail tests that are now passing: we're now back to 100% passing tests on the Win7 test slaves. Waiting for the WinXP results...
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-07-18 06:43:13 PDT
Same on WinXP. But both on Win7 and WinXP, there is a crash that we need to take care of:

CPU: x86
     GenuineIntel family 6 model 23 stepping 10
     2 CPUs

Crash reason:  EXCEPTION_ACCESS_VIOLATION_READ
Crash address: 0x3c4

Thread 0 (crashed)
 0  libGLESv2.dll!gl::ProgramBinary::getDxFrontCCWLocation() [ProgramBinary.cpp:65e28130452e : 2756 + 0x0]
    eip = 0x71f1e802   esp = 0x0023c564   ebp = 0x0023c588   ebx = 0x131f91d0
    esi = 0x00000000   edi = 0x00000004   eax = 0x1f4852b8   ecx = 0x00000000
    edx = 0x0023c570   efl = 0x00210202
    Found by: given as instruction pointer in context
 1  libGLESv2.dll!gl::Context::applyState(unsigned int) [Context.cpp:65e28130452e : 2054 + 0x9]
    eip = 0x71f03a46   esp = 0x0023c568   ebp = 0x0023c588
    Found by: stack scanning
 2  libGLESv2.dll!gl::Context::drawArrays(unsigned int,int,int,int) [Context.cpp:65e28130452e : 3013 + 0x7]
    eip = 0x71f07819   esp = 0x0023c590   ebp = 0x0023c5a8
    Found by: call frame info
 3  libGLESv2.dll!glDrawArrays [libGLESv2.cpp:65e28130452e : 2092 + 0x10]
    eip = 0x71f14de3   esp = 0x0023c5b0   ebp = 0x0023c5e4
    Found by: call frame info
 4  xul.dll!mozilla::gl::GLContext::raw_fDrawArrays(unsigned int,int,int) [GLContext.h:65e28130452e : 2167 + 0xe]
    eip = 0x6b1a9341   esp = 0x0023c5ec   ebp = 0x0023c600
    Found by: call frame info
 5  xul.dll!mozilla::WebGLContext::DrawArrays(unsigned int,int,int) [WebGLContextGL.cpp:65e28130452e : 1767 + 0x20]
    eip = 0x6b1bb335   esp = 0x0023c608   ebp = 0x0023c634
    Found by: call frame info
 6  xul.dll!mozilla::dom::WebGLRenderingContextBinding::drawArrays [WebGLRenderingContextBinding.cpp:65e28130452e : 2237 + 0x10]
    eip = 0x6bd23593   esp = 0x0023c63c   ebp = 0x0023c660
    Found by: call frame info
 7  mozjs.dll!js::CallJSNative(JSContext *,int (*)(JSContext *,unsigned int,JS::Value *),JS::CallArgs const &) [jscntxtinlines.h:65e28130452e : 385 + 0xe]
    eip = 0x6a7045c6   esp = 0x0023c668   ebp = 0x0023c690
    Found by: call frame info
 8  mozjs.dll!js::InvokeKernel(JSContext *,JS::CallArgs,js::MaybeConstruct) [jsinterp.cpp:65e28130452e : 344 + 0x14]
    eip = 0x6a7057ec   esp = 0x0023c698   ebp = 0x0023c6d8
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-07-18 11:32:31 PDT
Visual Studio disagrees and points to this stack instead:

 	msvcr100d.dll!unaligned_memcmp(const unsigned char * bLHS, const unsigned char * bRHS, unsigned int siz)  + 0x28d bytes	C
 	msvcr100d.dll!memcmp(const void * lhs, const void * rhs, unsigned int siz)  + 0x19c bytes	C
>	libGLESv2.dll!gl::Context::applyRenderTarget(bool ignoreViewport)  Line 1989 + 0x31 bytes	C++
 	libGLESv2.dll!gl::Context::drawArrays(unsigned int mode, int first, int count, int instances)  Line 3015	C++
 	libGLESv2.dll!glDrawArrays(unsigned int mode, int first, int count)  Line 2094	C++
 	xul.dll!mozilla::gl::GLContext::raw_fDrawArrays(unsigned int mode, int first, int count)  Line 2215	C++
 	xul.dll!mozilla::gl::GLContext::fDrawArrays(unsigned int mode, int first, int count)  Line 1202	C++
 	xul.dll!mozilla::WebGLContext::DrawArrays(unsigned int mode, int first, int count)  Line 1769	C++

really weird, these pointers seems completely valid for all I can see...
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-07-18 12:54:19 PDT
Fixed the crash locally with a ANGLE patch. The correct stack was the one from comment 5. Don't know why MSVC got it all wrong (it actually gave me a different stack everytime even though the crash location was the same). Maybe stack smashing.

Try push with ANGLE patch:
https://tbpl.mozilla.org/?tree=Try&rev=1502a8cbe063
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-07-18 13:06:45 PDT
Filed ANGLE bug 351:
https://code.google.com/p/angleproject/issues/detail?id=351
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-07-18 15:15:35 PDT
Created attachment 643618 [details] [diff] [review]
update the makefiles

Not going to ask you to review everything, just the parts where reviewing makes sense i.e. parts that don't come from upstream, and that aren't just rebased patches.

This updates the makefiles; the nontrivial parts are:
 - removed a lot of cpp files from the libEGL build, indeed libEGL doesn't need a shader compiler; you can easily trust that part as a mistake there would result in failure to link libEGL;
 - switched from the NSPR backend to the native Windows/POSIX backends for the TLS stuff (ossource_*.cpp). I had a chat with Vlad about that and he confirmed that there wasn't a good reason to use the NSPR backend besides that it made things easier in the makeflles (it didn't require ifdef'ing). Using the NSPR backend makes me nervous as 1) we're the only ones to use it 2) TLS performance can easily be critical (no idea whether it is in ANGLE's case) and 3) we know that NSPR TLS helpers have performance issues (or so I've been told by jrmuizel/ehsan IIRC --- leading to the mfbt/ThreadLocal stuff as a better replacement).

-> We must not forget to tell the ANGLE devs that the NSPR backend is no longer useful.
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2012-07-18 15:18:05 PDT
Created attachment 643621 [details] [diff] [review]
rename the preprocessor's Diagnostics.cpp file

Because our build system can't handle two different files in two directories having the same filename. Note we already do this for debug.*. This time it's easier as there only is a cpp file to rename (the corresponding .h is only #included from the same directory, is not exported, so it doesn't cause any problem).
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2012-07-18 15:18:48 PDT
Created attachment 643622 [details] [diff] [review]
rename the preprocessor's DirectiveHandler.cpp file

Same.
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2012-07-18 15:20:06 PDT
Created attachment 643624 [details] [diff] [review]
flip the WebGL canvas when using share handles in d3d10 layers

ANGLE recently changed their y-orientation.
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-07-18 15:21:21 PDT
Created attachment 643626 [details] [diff] [review]
fix ANGLE crash on null programBinary

This is the fix for the ANGLE bug discussed above. Pending review @ ANGLE, worth reviewing independently here so we can land asap.
Comment 14 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-18 15:48:02 PDT
Comment on attachment 643626 [details] [diff] [review]
fix ANGLE crash on null programBinary

watch indentation/tabs in this patch
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2012-07-19 08:21:02 PDT
Oranges on latest push are ANGLE issue 352:
https://code.google.com/p/angleproject/issues/detail?id=352

It's a conformance regression; I think we still want the ANGLE update as 1) the test failure is a common one in many drivers and not something that kills real-world content and 2) if it's actually a bug in ANGLE, it's likely to get fixed soon.
Comment 16 Benoit Jacob [:bjacob] (mostly away) 2012-07-19 08:26:59 PDT
https://tbpl.mozilla.org/?tree=Try&rev=506c39eecfd8
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2012-07-19 08:27:10 PDT
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #14)
> Comment on attachment 643626 [details] [diff] [review]
> fix ANGLE crash on null programBinary
> 
> watch indentation/tabs in this patch

Oops, right.
Comment 18 Benoit Jacob [:bjacob] (mostly away) 2012-07-19 11:32:19 PDT
Comment on attachment 643626 [details] [diff] [review]
fix ANGLE crash on null programBinary

As discussed on ANGLE issues 351/352, this isn't the proper fix, and is causing the conformance regression we're observing. Waiting on proper fix from ANGLE devs.
Comment 19 Benoit Jacob [:bjacob] (mostly away) 2012-07-24 15:14:32 PDT
ANGLE bug is fixed in r1242. New try:
https://tbpl.mozilla.org/?tree=Try&rev=6ce8015a0e6a
Comment 20 Jeff Gilbert [:jgilbert] 2012-07-24 15:24:57 PDT
Comment on attachment 643618 [details] [diff] [review]
update the makefiles

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

Some questions.

::: gfx/angle/Makefile.in
@@ +35,5 @@
>  CPPSRCS = \
> +        Diagnostics.cpp \
> +        PreprocessorDiagnostics.cpp \
> +        DirectiveHandler.cpp \
> +        PreprocessorDirectiveHandler.cpp \

Shouldn't we be using "Preprocessor[Diagnostics,DirectiveHandler].cpp", since we just renamed these files?

::: gfx/angle/src/libEGL/Makefile.in
@@ +5,5 @@
> +
> +DEPTH    = ../../../..
> +topsrcdir  = @top_srcdir@
> +srcdir    = @srcdir@
> +VPATH    = @srcdir@

Alignment?

@@ +31,5 @@
> +
> +LOCAL_INCLUDES = \
> + -I$(srcdir)/../../include \
> + -I$(srcdir)/.. \
> + -I"$(MOZ_DIRECTX_SDK_PATH)/include" \

Are these quotes necessary?

::: gfx/angle/src/libGLESv2/Makefile.in
@@ +5,5 @@
> +
> +DEPTH                = ../../../..
> +topsrcdir        = @top_srcdir@
> +srcdir                = @srcdir@
> +VPATH                = @srcdir@

Any chance we could get these lined up nicer?

@@ +31,5 @@
> +
> +LOCAL_INCLUDES = \
> + -I$(srcdir)/../../include \
> + -I$(srcdir)/.. \
> + -I"$(MOZ_DIRECTX_SDK_PATH)/include" \

This format seems weird. Based on the previous lines, it looks like we should prefer:
 -I$(MOZ_DIRECTX_SDK_PATH)/include

(without quotes)

@@ +46,5 @@
> +# Translator/compiler first
> +
> +CPPSRCS = \
> +        Diagnostics.cpp \
> +        PreprocessorDiagnostics.cpp \

Shouldn't this only have one of Diagnostics or PreprocessorDiagnostics?

@@ +48,5 @@
> +CPPSRCS = \
> +        Diagnostics.cpp \
> +        PreprocessorDiagnostics.cpp \
> +        DirectiveHandler.cpp \
> +        PreprocessorDirectiveHandler.cpp \

DirectiveHandler vs PreprocessorDirectiveHandler?
Comment 21 Benoit Jacob [:bjacob] (mostly away) 2012-07-24 15:29:54 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #20)
> Comment on attachment 643618 [details] [diff] [review]
> update the makefiles
> 
> Review of attachment 643618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some questions.
> 
> ::: gfx/angle/Makefile.in
> @@ +35,5 @@
> >  CPPSRCS = \
> > +        Diagnostics.cpp \
> > +        PreprocessorDiagnostics.cpp \
> > +        DirectiveHandler.cpp \
> > +        PreprocessorDirectiveHandler.cpp \
> 
> Shouldn't we be using "Preprocessor[Diagnostics,DirectiveHandler].cpp",
> since we just renamed these files?

In ANGLE there are two Diagnostics.cpp files. Our build system doesn't like that, so we renamed one of them to PreprocessorDiagnostics.cpp. We want both listed here in CPPRSCS. Same for DirectiveHandler.cpp.

> 
> ::: gfx/angle/src/libEGL/Makefile.in
> @@ +5,5 @@
> > +
> > +DEPTH    = ../../../..
> > +topsrcdir  = @top_srcdir@
> > +srcdir    = @srcdir@
> > +VPATH    = @srcdir@
> 
> Alignment?

Thanks

> 
> @@ +31,5 @@
> > +
> > +LOCAL_INCLUDES = \
> > + -I$(srcdir)/../../include \
> > + -I$(srcdir)/.. \
> > + -I"$(MOZ_DIRECTX_SDK_PATH)/include" \
> 
> Are these quotes necessary?

I think the quotest are needed at least around $(MOZ_DIRECTX_SDK_PATH) for the case when it expands to a string containing spaces, which is the typical case (at least "June 2010").

> 
> ::: gfx/angle/src/libGLESv2/Makefile.in
> @@ +5,5 @@
> > +
> > +DEPTH                = ../../../..
> > +topsrcdir        = @top_srcdir@
> > +srcdir                = @srcdir@
> > +VPATH                = @srcdir@
> 
> Any chance we could get these lined up nicer?

Thanks for catching that, yes.

> 
> @@ +31,5 @@
> > +
> > +LOCAL_INCLUDES = \
> > + -I$(srcdir)/../../include \
> > + -I$(srcdir)/.. \
> > + -I"$(MOZ_DIRECTX_SDK_PATH)/include" \
> 
> This format seems weird. Based on the previous lines, it looks like we
> should prefer:
>  -I$(MOZ_DIRECTX_SDK_PATH)/include
> 
> (without quotes)

Same comment as above about the need for double quotes here.
Comment 22 Jeff Gilbert [:jgilbert] 2012-07-24 15:31:45 PDT
Comment on attachment 643618 [details] [diff] [review]
update the makefiles

Ok, great. Take care of the alignment nits if you get a chance.
Comment 23 Benoit Jacob [:bjacob] (mostly away) 2012-07-24 18:48:49 PDT
Done.

Previous try was red because I had forgotten to hg add the new files from the ANGLE 1226 -> 1242 diff.

https://tbpl.mozilla.org/?tree=Try&rev=312e0cf9ad03

Note You need to log in before you can comment on or make changes to this bug.