Closed Bug 766796 Opened 9 years ago Closed 9 years ago

No particles in Spectrascade WebGL demo (regression)


(Core :: XPConnect, defect)

15 Branch
Windows 7
Not set



Tracking Status
firefox15 + verified
firefox16 + fixed


(Reporter: epinal99-bugzilla2, Assigned: bzbarsky)




(Keywords: regression)


(2 files, 1 obsolete file)

Open Spectrascade WebGL demo:
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:


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)
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?
Regression window(m-i)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120523083523
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120523093244

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
Blocks: 757526
Version: 16 Branch → 15 Branch
Wow! Thanks a lot for this bisecting. Looking.
The demo generates a lot of these:

[11:23:36.160] Error: WebGL: bufferSubData: negative offset @

Together with the fact that it's a regression from Bug 757526, it's quite clear what's happening there.
Here's the bufferSubData change made in bug 757526:

-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.
Can't easily debug on Windows at the moment due to bug 767006.
Depends on: 767006
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.
Component: Canvas: WebGL → XPConnect
QA Contact: canvas.webgl → xpconnect
Assignee: nobody → bzbarsky
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
Attachment #635655 - Flags: review?(peterv)
Whiteboard: [need review]
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:


  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.
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...
Attached patch Now with testSplinter Review
Attachment #635862 - Flags: review?(peterv)
Attachment #635655 - Attachment is obsolete: true
Attachment #635655 - Flags: review?(peterv)
Attachment #635862 - Flags: review?(peterv) → review+
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla16
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
String or UUID changes made by this patch: None.
Attachment #635862 - Flags: approval-mozilla-aurora?
And to deal with the Mac 10.5 bustage due to it not supporting WebGL.
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.
Attachment #635862 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
You need to log in before you can comment on or make changes to this bug.