Closed Bug 774755 Opened 12 years ago Closed 12 years ago

Update ANGLE to r1242

Categories

(Core :: Graphics: CanvasWebGL, defect)

Product:
Core
Component:
Graphics: CanvasWebGL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17

People

(Reporter: bjacob, Assigned: bjacob)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

update the makefiles
(deleted), patch
jgilbert
: review+
Details | Diff | Splinter Review
rename the preprocessor's Diagnostics.cpp file
(deleted), patch
jgilbert
: review+
Details | Diff | Splinter Review
rename the preprocessor's DirectiveHandler.cpp file
(deleted), patch
jgilbert
: review+
Details | Diff | Splinter Review
flip the WebGL canvas when using share handles in d3d10 layers
(deleted), patch
jgilbert
: review+
Details | Diff | Splinter Review
fix ANGLE crash on null programBinary
(deleted), patch
Details | Diff | Splinter Review 
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
Attached patch update the makefiles (deleted) — — Splinter Review 
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.
Attachment #643618 - Flags: review?(jgilbert)
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).
Attachment #643621 - Flags: review?(jgilbert)
Same.
Attachment #643622 - Flags: review?(jgilbert)
ANGLE recently changed their y-orientation.
Attachment #643624 - Flags: review?(jgilbert)
This is the fix for the ANGLE bug discussed above. Pending review @ ANGLE, worth reviewing independently here so we can land asap.
Attachment #643626 - Flags: review?(jgilbert)
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.
Attachment #643626 - Flags: review?(jgilbert)
Summary: Update ANGLE to r1226 → Update ANGLE to r1242
Attachment #643621 - Flags: review?(jgilbert) → review+
Attachment #643622 - Flags: review?(jgilbert) → review+
Attachment #643624 - Flags: review?(jgilbert) → review+
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.
Attachment #643618 - Flags: review?(jgilbert) → review+
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
Depends on: 795117
Depends on: 798919
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: