The default bug view has changed. See this FAQ.

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

(Blocks: 1 bug)

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

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
(Assignee)

Comment 1

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

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Alright, libEGL shrinks from 84k to 58k... zip archive size shrinks by 16k.
(Assignee)

Comment 4

5 years ago
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...
(Assignee)

Comment 5

5 years ago
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
(Assignee)

Comment 6

5 years ago
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...
(Assignee)

Comment 7

5 years ago
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
(Assignee)

Comment 8

5 years ago
Filed ANGLE bug 351:
https://code.google.com/p/angleproject/issues/detail?id=351
(Assignee)

Comment 9

5 years ago
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.
Attachment #643618 - Flags: review?(jgilbert)
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).
Attachment #643621 - Flags: review?(jgilbert)
Created attachment 643622 [details] [diff] [review]
rename the preprocessor's DirectiveHandler.cpp file

Same.
Attachment #643622 - Flags: review?(jgilbert)
Created attachment 643624 [details] [diff] [review]
flip the WebGL canvas when using share handles in d3d10 layers

ANGLE recently changed their y-orientation.
Attachment #643624 - Flags: review?(jgilbert)
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.
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.
https://tbpl.mozilla.org/?tree=Try&rev=506c39eecfd8
(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)
ANGLE bug is fixed in r1242. New try:
https://tbpl.mozilla.org/?tree=Try&rev=6ce8015a0e6a
(Assignee)

Updated

5 years ago
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
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
Assignee: nobody → bjacob
Target Milestone: --- → mozilla17
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 795117

Updated

5 years ago
Depends on: 798919
You need to log in before you can comment on or make changes to this bug.