Last Comment Bug 766796 - No particles in Spectrascade WebGL demo (regression)
: No particles in Spectrascade WebGL demo (regression)
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: 15 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla16
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
http://www.jeshua.me/spectrascade/sc
Depends on: 767006
Blocks: 757526
  Show dependency treegraph
 
Reported: 2012-06-20 17:21 PDT by Loic
Modified: 2012-07-31 07:43 PDT (History)
10 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
fixed


Attachments
spectrascade snapshot with particles (222.59 KB, image/jpeg)
2012-06-20 17:24 PDT, Loic
no flags Details
Make IDL conversions to 64-bit ints treat NaN and Infinity as 0 instead of whatever the compiler decides to do in that undefined-behavior case. (2.49 KB, patch)
2012-06-22 00:40 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
Now with test (4.35 KB, patch)
2012-06-22 12:26 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
peterv: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Loic 2012-06-20 17:21:05 PDT
Open Spectrascade WebGL demo: http://www.jeshua.me/spectrascade/sc
You should see a small gray 3D cube with a stream of particles around it.
In FF16, there are no particles but the rest of the demo is OK.

Mozregression range:

m-c
2012-05-24
2012-05-25
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f43e8d300f21&tochange=1dd0c5c6d9fd

m-i
2012-05-23
2012-05-24
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=59ec4eabd9ce&tochange=1865549541b7
Comment 1 Loic 2012-06-20 17:24:20 PDT
Created attachment 635146 [details]
spectrascade snapshot with particles
Comment 2 Jeff Gilbert [:jgilbert] 2012-06-20 19:02:36 PDT
I don't see any particles on FF 15 or 16 on Win7.
I do see what looks like five duplicate warnings in the Error Console per frame: "bufferSubData: negative offset." (at least, until it hits the 32-warning limit we added)
Comment 3 Jeff Gilbert [:jgilbert] 2012-06-20 19:05:38 PDT
You also seem to be triggering a SetDimensions every frame. Are you setting the width and height to the same thing every frame, or something?
Comment 4 Alice0775 White 2012-06-21 02:43:27 PDT
Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7ae630f43357
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120523083523
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5f14275bb276
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120523093244
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7ae630f43357&tochange=5f14275bb276


Inlocal build
Last Good: 7ffffcb45b94
First Bad: dd6c4f6a2448

Triggered by:
dd6c4f6a2448	Benoit Jacob — Bug 757526 - Use stdint instead of PRInt types in WebGL implementation - r=Ms2ger
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-06-21 08:04:02 PDT
Wow! Thanks a lot for this bisecting. Looking.
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-06-21 08:24:50 PDT
The demo generates a lot of these:

[11:23:36.160] Error: WebGL: bufferSubData: negative offset @ http://www.jeshua.me/spectrascade/js/spidergl.js:4748

Together with the fact that it's a regression from Bug 757526, it's quite clear what's happening there.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-06-21 08:29:42 PDT
Here's the bufferSubData change made in bug 757526:


https://hg.mozilla.org/mozilla-central/rev/dd6c4f6a2448#l2.65

-WebGLContext::BufferSubData(PRInt32 target, PRInt32 offset, const JS::Value& data, JSContext *cx)
+WebGLContext::BufferSubData(WebGLenum target, WebGLintptr offset, const JS::Value& data, JSContext *cx)

This is indeed a functional change, but as far as I can see, it is fixing a conformance bug, not introducing one. Need further debugging, but at the moment it looks as if the demo is doing invalid bufferSubData calls. Not yet sure though that that is related to the rendering issue.
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-06-21 09:03:09 PDT
Can't easily debug on Windows at the moment due to bug 767006.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-21 15:19:03 PDT
So the second argument being passed in is an object.  Specifically, a WebGLBuffer.

This lands in xpc::ValueToInt64, which converts it to a double.  It gets back NaN.

Per spec this _should_ get turned into 0, but xpc::ValueToInt64 does static_cast<int64_t>(doubleval), which gets us 0x8000000000000000 (the bit-pattern of the NaN), but treated as a signed 64-bit int.  I have no idea why it does that, bit-pattern thing, exactly.

In any case, that's a negative value, treated as a signed 64-bit int, and things break.

Now the page is of course totally broken in terms of what it passes to that second argument.  But we need to fix the bug in xpc::ValueToInt64.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-22 00:40:15 PDT
Created attachment 635655 [details] [diff] [review]
Make IDL conversions to 64-bit ints treat NaN and Infinity as 0 instead of whatever the compiler decides to do in that undefined-behavior case.

Benoit, what's a good way of testing this?  Should be easy to reproduce by just passing NaN to any WebGL method that takes signed 64-bit ints and requires them to be nonnegative, without this patch
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2012-06-22 08:18:16 PDT
We can simply take any WebGL method that takes a 64bit integer and allows to test, from JavaScript, whether that integer is exactly zero. bufferSubData is such a method and is probably the one that makes this easiest. Indeed, the WebGL spec says: "If the data would be written past the end of the buffer object an INVALID_VALUE error is generated." It also inherits the GL requirement to generate an error if the offset is negative. So it's quite simple: create a buffer of N bytes, and try to update N bytes at a certain offset, this will generate an error if the offset is nonzero. Like this:

  assert(!gl.getError());

  var b = gl.createBuffer();
  gl.bindBuffer(gl.ARRAY_BUFFER, b);

  var a = new Float32Array(1);
  gl.bufferData(gl.ARRAY_BUFFER, a, gl.STATIC_DRAW);

  var nan = 0/0;
  gl.bufferSubData(gl.ARRAY_BUFFER, nan, a);

  assert(!gl.getError()); // this is the test

But I'm not sure that we want to take this into the WebGL conformance test suite. The last time that I checked in such a general DOM/JS bindings test in the WebGL conformance test suite, I got a well-founded remark that this didn't really fit in there. Isn't there a "DOM/JS bindings conformance test suite"? Would fit better in such a place. If there isn't then maybe we could at least rationalize this effort by adding a dedicated test page for these things to the WebGL conformance tests.
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-22 09:37:10 PDT
Oh, I was just going to add this to our tree.  There is no general "DOM/JS bindings test suite" yet, though I'm hoping there will be one at some point...
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-22 12:26:13 PDT
Created attachment 635862 [details] [diff] [review]
Now with test
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-25 20:43:06 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/82849fb54cc0
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-25 20:45:08 PDT
Comment on attachment 635862 [details] [diff] [review]
Now with test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 757526 
User impact if declined: Some WebGL things apparently won't work correctly
Testing completed (on m-c, etc.): Page that shows this bug
Risk to taking this patch (and alternatives if risky): Low-risk: just converts
   NaN to 0 when converting to int, as the spec requires, instead of converting
   to garbage.  Only affects conversion to 64-bit ints, which are very rare in
   IDL.
String or UUID changes made by this patch: None.
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-25 22:11:16 PDT
And https://hg.mozilla.org/integration/mozilla-inbound/rev/421b653f33dc to deal with the Mac 10.5 bustage due to it not supporting WebGL.
Comment 18 Alex Keybl [:akeybl] 2012-06-27 15:39:43 PDT
Comment on attachment 635862 [details] [diff] [review]
Now with test

[Triage Comment]
Low risk fix that brings us closer to spec. Approved for Aurora 15.
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-27 21:03:11 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/c697cdebddc9 for the roll-up patch.
Comment 20 Paul Silaghi, QA [:pauly] 2012-07-31 07:43:31 PDT
Able to see the issue on Nightly 2012-06-01. The particles are seen on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0b2. Verified fixed.

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