Closed Bug 1237748 Opened 6 years ago Closed 5 years ago

Firefox deletes a nonexistent renderbuffer at GL context startup

Categories

(Core :: Canvas: WebGL, defect)

x86_64
Windows 10
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox46 --- affected
firefox51 --- wontfix
firefox53 --- fixed

People

(Reporter: jujjyl, Assigned: kvark)

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files, 4 obsolete files)

CodeXL reports the following: https://dl.dropboxusercontent.com/u/40949268/Bugs/codexl_errors.png

"Warning: The debugged program deleted a render buffer that does not exist. Render buffer name: 1"

Nothing bad will happen directly due to this (GL deletes silently ignore names that are not valid), but might be worth checking out to see if there's some unexpected code flow going on.
jgilbert, can you comment to the bug?
Flags: needinfo?(jgilbert)
Whiteboard: [gfx-noted]
Jerry, can you take a look?
Flags: needinfo?(jgilbert) → needinfo?(hshih)
Hi Jukka,

What's your testing environment? And what's your testing page for webgl?

I use CodeXL to debug my nightly build(my pc is ubuntu with nvidia card). But all webgl shows abnormal result. I can make sure the webgl page is fine if I just open with the standalone nightly.

Could you share your CodeXL project setting?
Flags: needinfo?(hshih) → needinfo?(jujjyl)
Attached file codexl_log.txt
My STR is

1. On Firefox Nightly on Windows, set the prefs

browser.tabs.remote.autostart.2;false
webgl.disable-angle;true

2. Open CodeXL and create new project, pass it executable name C:/Program Files/Nightly/firefox.exe and change nothing else.

3. In dropdown menu, choose Debug->Start Debugging(F5)

4. A dialog like https://dl.dropboxusercontent.com/u/40949268/dump/amd_codexl_warning.png might show up. I followed the instructions there and copied C:\Program Files (x86)\AMD\CodeXL\spies64\*.dll to C:/Program Files/Nightly/

5. Navigate to e.g. https://dl.dropboxusercontent.com/u/40949268/emcc/2015-08-25-emunittest_0.4-AngryBots-u5.1.3f1_hg-e1.34.6-release-prof/index.html

And then look at the CodeXL report. Attached the report I got after doing the above.
Flags: needinfo?(jujjyl)
Hi Jukka,
I follow the step 1 to 3, but I don't see the message in step 4.
And I still can't run the page in step 5.

Could you please try another webgl sample[1] and check log again?

[1]
http://webglsamples.org/
Flags: needinfo?(jujjyl)
Attached file codexl_log_2.txt
In the page you posted: 

1. - 4. Same steps as above.
5. Navigate to http://webglsamples.org/
6. Close browser window and profiling.

Pasted CodeXL log. In particular, it reads

Debug String: Warning: The debugged program deleted a render buffer that does not exist. Render Buffer name: 2
Debug String: Warning: The debugged program deleted a render buffer that does not exist. Render Buffer name: 2
Debug String: Warning: The debugged program deleted a render buffer that does not exist. Render Buffer name: 6
Debug String: Warning: The debugged program deleted a render buffer that does not exist. Render Buffer name: 4
Checking for OpenGL memory leaks - Application exit
Memory Leak: PBuffer 0, 1 were not deleted before application termination
Flags: needinfo?(jujjyl)
Blocks: 1261597
Sorry for the above noise, "Clone this bug" is silly and auto-adds dependency relationship, which doesn't exist for these two.
No longer blocks: 1261597
From comment 6, marked down separate bug entry https://bugzilla.mozilla.org/show_bug.cgi?id=1261597 for the memory leak report.

Also marking as 64-bit Windows 10, not sure if this is specific to that, but that is what this was tested on.
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Attached patch webgl-rbuf-delete.patch (obsolete) — Splinter Review
Assignee: nobody → kvark
Status: NEW → ASSIGNED
Attachment #8821558 - Flags: review?(jgilbert)
Comment on attachment 8821558 [details] [diff] [review]
webgl-rbuf-delete.patch

Review of attachment 8821558 [details] [diff] [review]:
-----------------------------------------------------------------

Non-existent but non-zero?
If non-zero but non-existent, we should determine why, because that really shouldn't happen.

The expected behavior here is to either delete valid buffers, or 0s. (which is valid)
We definitely shouldn't be asking for GL to delete a non-zero invalid handle.
Attachment #8821558 - Flags: review?(jgilbert) → review-
Jeff, the depth and stencil surfaces were deleted separately (have you looked at the patch?), but allocated via a single depth-stencil surface. So it would try to delete it twice, but the second time the renderbuffer handle is non-zero and invalid.
Flags: needinfo?(jgilbert)
(In reply to Dzmitry Malyshau from comment #11)
> Jeff, the depth and stencil surfaces were deleted separately (have you
> looked at the patch?), but allocated via a single depth-stencil surface. So
> it would try to delete it twice, but the second time the renderbuffer handle
> is non-zero and invalid.

Of course I looked at the patch when I reviewed it. :) Please leave a comment as to why we're doing the equality check. This reasoning is not obvious from your patch.
Flags: needinfo?(jgilbert)
Attached patch webgl-rbuf-delete2.patch (obsolete) — Splinter Review
Added a few comments in the updated patch now
Attachment #8821558 - Attachment is obsolete: true
Attachment #8822178 - Flags: review?(jgilbert)
Comment on attachment 8822178 [details] [diff] [review]
webgl-rbuf-delete2.patch

Review of attachment 8822178 [details] [diff] [review]:
-----------------------------------------------------------------

Up to you.

::: gfx/gl/GLScreenBuffer.cpp
@@ +878,3 @@
>      GLuint rbs[] = {
>          mColorMSRB,
>          mDepthRB,

It's probably nicest if you leave a 3rd entry here, but make it:
`(mStencilRB != mDepthRB) ? mStencilRB : 0` // Don't double-delete DEPTH_STENCIL RBs.

@@ +883,5 @@
>      mGL->fDeleteFramebuffers(1, &fb);
> +    mGL->fDeleteRenderbuffers(2, rbs);
> +
> +    // Avoid deleting the surface twice if it has both depth and stencil
> +    if (mStencilRB && mStencilRB != mDepthRB) {

Don't bother checking bool(mStencilRB).

@@ +971,3 @@
>  
>      mGL->fDeleteFramebuffers(1, &fb);
> +    if (mDepthRB) {

Don't bother checking bool(mDepthRB).

@@ +973,5 @@
> +    if (mDepthRB) {
> +        mGL->fDeleteRenderbuffers(1, &mDepthRB);
> +    }
> +    // Avoid deleting the surface twice if it has both depth and stencil
> +    if (mStencilRB && mStencilRB != mDepthRB) {

Don't bother checking bool(mStencilRB).
Attachment #8822178 - Flags: review?(jgilbert) → review+
Attached patch webgl-rbuf-delete3.patch (obsolete) — Splinter Review
Agreed, attached the updated patch (version 3) now
Attachment #8822178 - Attachment is obsolete: true
Attachment #8822297 - Flags: review?(jgilbert)
Comment on attachment 8822297 [details] [diff] [review]
webgl-rbuf-delete3.patch

Review of attachment 8822297 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLScreenBuffer.cpp
@@ +878,4 @@
>      GLuint rbs[] = {
>          mColorMSRB,
>          mDepthRB,
> +        mStencilRB != mDepthRB ? mStencilRB : 0, // Don't double-delete DEPTH_STENCIL RBs.

Paren around trinary conditionals. Operator precedence guarantees this, but do it for readability.
Attachment #8822297 - Flags: review?(jgilbert) → review+
Attached patch webgl-rbuf-delete4.patch (obsolete) — Splinter Review
Fixed now
Attachment #8822297 - Attachment is obsolete: true
Attachment #8822426 - Flags: review?(jgilbert)
Comment on attachment 8822426 [details] [diff] [review]
webgl-rbuf-delete4.patch

Review of attachment 8822426 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect, thanks!
Attachment #8822426 - Flags: review?(jgilbert) → review+
Keywords: checkin-needed
Please attach a patch that contains the necessary patch metadata (author, useful commit message, etc) for landing.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Flags: needinfo?(kvark)
Keywords: checkin-needed
Attachment #8822426 - Attachment is obsolete: true
Flags: needinfo?(kvark)
Comment on attachment 8825113 [details] [diff] [review]
Avoid deleting the stencil RB if it's the same as the depth RB.

No need to re-request review if you're just adding metadata to the patch. Thanks!
Attachment #8825113 - Flags: review?(jgilbert)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1940969b24fb
Avoid deleting the stencil RB if it's the same as the depth RB. r=jgilbert
Thanks :RyanVM!
Do I still mark the reviewed attachment as obsolete in this case?
Flags: needinfo?(ryanvm)
Yeah, that's fine (the review stays on the obsolete patch, so that information isn't lost).
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/mozilla-central/rev/1940969b24fb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.