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

Oh yes, I also wiped most of the CPPSRCS from libEGL/Makefile.in, so we should have a significantly smaller libEGL on Windows now.

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.

Alright, libEGL shrinks from 84k to 58k... zip archive size shrinks by 16k.

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...

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

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...

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

Filed ANGLE bug 351: https://code.google.com/p/angleproject/issues/detail?id=351

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.

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).

Same.

ANGLE recently changed their y-orientation.

This is the fix for the ANGLE bug discussed above. Pending review @ ANGLE, worth reviewing independently here so we can land asap.

Comment on attachment 643626 [details] [diff] [review] fix ANGLE crash on null programBinary watch indentation/tabs in this patch

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.

(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 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.

ANGLE bug is fixed in r1242. New try: https://tbpl.mozilla.org/?tree=Try&rev=6ce8015a0e6a

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?

(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 on attachment 643618 [details] [diff] [review] update the makefiles Ok, great. Take care of the alignment nits if you get a chance.

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/15e240011e10 http://hg.mozilla.org/integration/mozilla-inbound/rev/c4a4804b2420 http://hg.mozilla.org/integration/mozilla-inbound/rev/2592d31d2f0d http://hg.mozilla.org/integration/mozilla-inbound/rev/41466cf61ec6 http://hg.mozilla.org/integration/mozilla-inbound/rev/2ab608e34bd0 http://hg.mozilla.org/integration/mozilla-inbound/rev/5e6ca15261af http://hg.mozilla.org/integration/mozilla-inbound/rev/c2c60b40fa7f http://hg.mozilla.org/integration/mozilla-inbound/rev/39c6cd95de25

https://hg.mozilla.org/mozilla-central/rev/15e240011e10 https://hg.mozilla.org/mozilla-central/rev/c4a4804b2420 https://hg.mozilla.org/mozilla-central/rev/2592d31d2f0d https://hg.mozilla.org/mozilla-central/rev/41466cf61ec6 https://hg.mozilla.org/mozilla-central/rev/2ab608e34bd0 https://hg.mozilla.org/mozilla-central/rev/5e6ca15261af https://hg.mozilla.org/mozilla-central/rev/c2c60b40fa7f https://hg.mozilla.org/mozilla-central/rev/39c6cd95de25