Closed Bug 1436190 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: kvark, Assigned: kvark)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → kvark
Attached patch capture-fix.patch (obsolete) — Splinter Review
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.
Attachment #8948812 - Flags: review?(nical.bugzilla)
Hmm, interesting. Linux is hitting assertion "info.mPendingCount > 0" with the patch. I wonder why the behavior is different on Windows.
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).
Component: Developer Tools → Graphics: WebRender
Product: Firefox → Core
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?
Flags: needinfo?(sotaro.ikeda.g)
Attached patch capture-fix.patch (obsolete) — Splinter Review
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.
Attachment #8948812 - Attachment is obsolete: true
Attachment #8948812 - Flags: review?(nical.bugzilla)
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8949277 - Flags: review?(nical.bugzilla)
Attached patch capture-fix.patch (obsolete) — Splinter Review
Now tested (positively) on Windows 7 and Linux.
Also fixes Bug 1433932.
Attachment #8949277 - Attachment is obsolete: true
Attachment #8949277 - Flags: review?(nical.bugzilla)
Attachment #8949283 - Flags: review?(nical.bugzilla)
(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.
Attachment #8949283 - Flags: review?(nical.bugzilla) → review+
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.
The same patch but without the path checking (moving to bug 1433932).
Attachment #8949283 - Attachment is obsolete: true
Attachment #8949360 - Flags: review?(bugmail)
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.
Attachment #8949360 - Flags: review?(bugmail) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c590c7dac39
Fixed render frame notification in wake_up(). r=nical
https://hg.mozilla.org/mozilla-central/rev/2c590c7dac39
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.