The default bug view has changed. See this FAQ.

No particles in Spectrascade WebGL demo (regression)

RESOLVED FIXED in Firefox 15

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Loic, Assigned: bz)

Tracking

({regression})

15 Branch
mozilla16
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15+ verified, firefox16+ fixed)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Updated

5 years ago
Keywords: regression
(Reporter)

Comment 1

5 years ago
Created attachment 635146 [details]
spectrascade snapshot with particles
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?

Comment 4

5 years ago
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
Blocks: 757526

Updated

5 years ago
tracking-firefox15: --- → ?
tracking-firefox16: --- → ?

Updated

5 years ago
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 @ 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.
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.
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
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
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:

  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.
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...
Created attachment 635862 [details] [diff] [review]
Now with test
Attachment #635862 - Flags: review?(peterv)
Attachment #635655 - Attachment is obsolete: true
Attachment #635655 - Flags: review?(peterv)
Attachment #635862 - Flags: review?(peterv) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/82849fb54cc0
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
   IDL.
String or UUID changes made by this patch: None.
Attachment #635862 - Flags: approval-mozilla-aurora?
And https://hg.mozilla.org/integration/mozilla-inbound/rev/421b653f33dc to deal with the Mac 10.5 bustage due to it not supporting WebGL.
https://hg.mozilla.org/mozilla-central/rev/82849fb54cc0
https://hg.mozilla.org/mozilla-central/rev/421b653f33dc
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
tracking-firefox15: ? → +
tracking-firefox16: ? → +
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+
http://hg.mozilla.org/releases/mozilla-aurora/rev/c697cdebddc9 for the roll-up patch.
status-firefox15: --- → fixed
status-firefox16: --- → fixed
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.
status-firefox15: fixed → verified
You need to log in before you can comment on or make changes to this bug.