792601 - Security Coverage: Inspect several WebGL files

Status: RESOLVED INCOMPLETE

(Core :: Graphics: CanvasWebGL, defect)

Description

[Christian Holler (:decoder)]

According to our analysis, the files content/canvas/src/WebGLContext.h, content/canvas/src/WebGLContextGL.cpp and content/canvas/src/WebGLContextValidate.cpp  have untested portions of code/functions and have both been patched one or more times in the last 6 months due to security problems. The coverage data for these files (from a try run with our main test suites) is here:

http://people.mozilla.org/~choller/coverage/mc-tests-all-20120903/content/canvas/src/

(Assertions, warnings and errors like out-of-memory conditions or those that cannot be triggered through content should be ignored).

File-specific comments: 

content/canvas/src/WebGLContext.h: Most interesting lines seem to be 2333, 2436, 2441, 2458, 2719, 2862, 2881, 2941 and 3253
content/canvas/src/WebGLContextGL.cpp: Several untested functions at 442, 599, 5128, 5184, 5199, 6003, etc., numerous lines in addition to that
content/canvas/src/WebGLContextValidate.cpp: Several untested functions at 369, 452, 501, additional lines at 688, 703, 716 and following

Please add appropriate tests and/or check the untested portions of code if it is possible and reasonable. Note that this test run was under Linux (on try). If any of this code cannot be tested on Linux, please let me know :)

Comment 1

[Kelsey Gilbert [:jgilbert]]

WebGLContext.h:
L2333 is UpperBoundNumSamplerUniforms, which is only hit if we're on Mesa GL drivers, which we don't use on Linux slaves.

A number of the rest in that file are only tested in the depth_texture extension, which we may not have the requisite extensions to run. We should probably check this.

WebGLContextGL.cpp:
L442 and 599 may be the TypedArray overloads, which might actually indeed not be tested properly.
L5128 and 5184 seem similar.
L5199 looks like we're missing tests for CompressedTexSubImage, which, since we're hitting CompressedTexImage, we should be also testing.
L5286 may be obsolete.

There are more, and there are some interesting things to look at. Is there an information available about how this data is being collected? Checking files like gfx/gl/GLContext, it looks like it isn't seeing any hits on many of the major GL calls.

Comment 2

[Christian Holler (:decoder)]

(In reply to Jeff Gilbert [:jgilbert] from comment #1)

> There are more, and there are some interesting things to look at. Is there
> an information available about how this data is being collected? Checking
> files like gfx/gl/GLContext, it looks like it isn't seeing any hits on many
> of the major GL calls.

Thanks for looking into the reports :) The data is generated by pushing a special configuration to try and have try run several of our testsuites (mochitests, reftest, crashtest, jsreftest, xpcshell tests and make check). As far as I know, all the GL tests are in mochitests, so those should have been run.

Comment 3

[Kelsey Gilbert [:jgilbert]]

WebGL conformance tests are in M1, and WebGL reftests are in R, Ru, and R2.

The issue is that when I look at:
http://people.mozilla.org/~choller/coverage/mc-tests-all-20120903/gfx/gl/GLContext.h.gcov.html
I see raw_fClear and fShaderSource (among others) are supposedly not ever used, which is not possible if WebGL ran and displayed at all. ReadPixels also looks like it's unused, which would mean no conformance tests ran. How is coverage determined? Sampling?

Comment 4

[Christian Holler (:decoder)]

The data we're looking at right now comes from the debug part of this try run:

https://tbpl.mozilla.org/?tree=Try&rev=4f4336dfec24

which shows that M1 and R ran and passed. The coverage is determined using regular gcov. It's highly unlikely that the code is covered although gcov shows it uncovered.

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Christian Holler (:decoder) from comment #4)
> which shows that M1 and R ran and passed. The coverage is determined using
> regular gcov. It's highly unlikely that the code is covered although gcov
> shows it uncovered.
Without raw_fClear, fShaderSource, and raw_fReadPixels, it's impossible that GL did anything useful.

For instance, the call at [1]:L787 should hit [2]:L1295, but doesn't appear to. Is gcov maybe not picking up inlined functions?

[1]: http://people.mozilla.org/~choller/coverage/mc-tests-all-20120903/content/canvas/src/WebGLContextGL.cpp.gcov.html
[2]: http://people.mozilla.org/~choller/coverage/mc-tests-all-20120903/gfx/gl/GLContext.h.gcov.html

Comment 6

[Christian Holler (:decoder)]

(In reply to Jeff Gilbert [:jgilbert] from comment #5)

> Is gcov maybe not picking up inlined functions?

That would be new to me, gcov has always been playing well with inlined functions if I remember correctly. I can try rerunning this without optimizations to make sure it's not related to that somehow. 

However, in [2]:L1334, the inlined function is picked up correctly. Are the calls maybe redirected to other code that's overloading this?

Maybe bjacob also has an idea.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Well that declaring inlined functions doesn't mean they actually get inlined. Is there maybe a max call stack depth? We call ForceDirtyFBOs() heigher in the call stack than most of these other functions. Also, [2]:L1337 (hah) doesn't show up in the function's definition at L1228.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Christian Holler (:decoder) from comment #6)
> (In reply to Jeff Gilbert [:jgilbert] from comment #5)
> 
> > Is gcov maybe not picking up inlined functions?
> 
> That would be new to me, gcov has always been playing well with inlined
> functions if I remember correctly. I can try rerunning this without
> optimizations to make sure it's not related to that somehow. 
> 
> However, in [2]:L1334, the inlined function is picked up correctly. Are the
> calls maybe redirected to other code that's overloading this?
> 
> Maybe bjacob also has an idea.

Nope; there's really no way those calls aren't being hit in GLContext if WebGL is being used or drawing anything, as jgilbert said.  It's missing something.

Comment 9

[Christian Holler (:decoder)]

I'll try to do a local run of the WebGL stuff in mochitest-1 with a gcov build and see if I can find the reason :)

Comment 10

[Christian Holler (:decoder)]

Alright, I think I found the reason. gcov is actually right but the way we compose the data is wrong. The header we're talking about seems to be moved to dist/include before being used to compile so gcov records this to objdir/dist/include/.. rather than gfx/gl. I'll check now if that data is included in the try push.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hmm.. -all- headers get moved to dist/include if they're exported (not just directory/module-local).  This sounds like it could be causing some incorrect numbers across the board?

Comment 12

[Christian Holler (:decoder)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #11)
> Hmm.. -all- headers get moved to dist/include if they're exported (not just
> directory/module-local).  This sounds like it could be causing some
> incorrect numbers across the board?

Yes, that's right. I'm fixing it right now by mapping the paths back to the original files in the tree and see if that looks better.

Comment 13

[Christian Holler (:decoder)]

I made an experimental try run with some code modifications in the script that tries to map dist/include/ headers back to their original sources in m-c:

http://people.mozilla.org/~choller/coverage/mc-tests-all-20120918-e4757379b99a/

Can you check if that makes more sense now at least for your subsystem? The algorithm might still not be perfect.

Comment 14

[Kelsey Gilbert [:jgilbert]]

(In reply to Christian Holler (:decoder) from comment #13)
> I made an experimental try run with some code modifications in the script
> that tries to map dist/include/ headers back to their original sources in
> m-c:
> 
> http://people.mozilla.org/~choller/coverage/mc-tests-all-20120918-
> e4757379b99a/
> 
> Can you check if that makes more sense now at least for your subsystem? The
> algorithm might still not be perfect.

At first glance, this looks much better. We seem to be hitting most functions in GLContext properly. I'll take a closer look at this later today.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Per discussion with decoder, these bugs are not likely to be relevant at this point (and the reports themselves are long gone). Any further work is better suited for new bugs at this point.