1436190 - Hang/crash on WR capturing with Ctrl+Shift+3

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P2)

Description

[Dzmitry Malyshau [:kvark]]

Getting a WR capture from the nightly has the correct keys but behaves undesirably by hanging on Windows and crashing on Linux. The problem appears to come from the recent change in `RenderNotifier` logic.

Comment 1

[Dzmitry Malyshau [:kvark]]

Attachment: capture-fix.patch

No need to increase the pending count (any more!) since the notification coming from the capture has `composite_needed: false`. Tested on Windows so far.

Comment 2

[Dzmitry Malyshau [:kvark]]

Hmm, interesting. Linux is hitting assertion "info.mPendingCount > 0" with the patch. I wonder why the behavior is different on Windows.

Comment 3

[Nicolas Silva [:nical]]

The devtools component is for stuff related to the debugger, inspector and other built-in tools for web developers. 

About the frame counting issue, Sotaro is probably the only person who fully understands the ins and outs of it and I don't have an idea off the top of my head. It's not uncommon for this type of issue to happen a lot more on a platform than another because it is timing sensitive (different test machines and other factors).

Comment 4

[Dzmitry Malyshau [:kvark]]

Sotaro, would you know why on Linux we have to increase the pending count (or otherwise hitting an assert) while on Windows we get a hang if we don't increase it?

Comment 5

[Dzmitry Malyshau [:kvark]]

Attachment: capture-fix.patch

Ok, let's try again. What I found is that FF expected a frame coming after `wake_up` call, which is incorrect, and that's what was bugging the capturing.
I'm yet to test this on anything but Linux, but it should be ready for review.

Comment 6

[Dzmitry Malyshau [:kvark]]

Attachment: capture-fix.patch

Now tested (positively) on Windows 7 and Linux.
Also fixes Bug 1433932.

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Dzmitry Malyshau [:kvark] from comment #4)
> Sotaro, would you know why on Linux we have to increase the pending count
> (or otherwise hitting an assert) while on Windows we get a hang if we don't
> increase it?

You already seems to address the problem. RenderThread::IncPendingFrameCount() is used to throttle WebRender's frame generation, since ideally WebRender's rendering should happen only after VSync, though actual throttling implementation relaxes a bit. If we tries to generate more frames, some of them are not rendered to Display and WebRender just spend more cpu/gpu resources and the more tasks just add more latency.

WebRenderBridgeParent expects that all frame generation happens by CompositeToTarget() like the following sequence.

WebRenderBridgeParent::CompositeToTarget()
->wr::RenderThread::IncPendingFrameCount(mApi->GetId()); // inc pending frame count
->txn.GenerateFrame(); // Add generate frame transaction.
->mApi->SendTransaction(txn);
->// ******** WebRender handle transaction **********
->new_document_ready() // call back from WebRender
->NewFrameReady()
->RenderThread::IncRenderingFrameCount(aWindowId);
->RenderThread::NewFrameReady(mozilla::wr::WindowId(aWindowId));
->RendererOGL::UpdateAndRender() // render frame
->RenderThread::DecPendingFrameCount()

Current implementation wake_up(&self) calls wr_notifier_new_frame_ready(self.window_id). The call breaks the way of managing PendingFrameCount. Then RenderThread::IncPendingFrameCount() in WebRenderAPI::Capture() also break PendingFrameCount. And attachment 8949283 [details] [diff] [review] addresses PendingFrameCount handling breaks.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

You should really split the patch into two and put the revision.txt changes on bug 1433932. In mozilla-central the convention is to not lump together fixes for unrelated bugs into the same commit.

Comment 9

[Dzmitry Malyshau [:kvark]]

Attachment: capture-notify.patch

The same patch but without the path checking (moving to bug 1433932).

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8949360 [details] [diff] [review]
capture-notify.patch

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

You can carry nical's r+ since he already reviewed it.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c590c7dac39
Fixed render frame notification in wake_up(). r=nical

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/2c590c7dac39