Closed
Bug 1436190
Opened 6 years ago
Closed 6 years ago
Hang/crash on WR capturing with Ctrl+Shift+3
Categories
(Core :: Graphics: WebRender, defect, P2)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: kvark, Assigned: kvark)
References
Details
Attachments
(1 file, 3 obsolete files)
4.57 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•6 years ago
|
Assignee: nobody → kvark
Assignee | ||
Comment 1•6 years ago
|
||
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)
Assignee | ||
Comment 2•6 years ago
|
||
Hmm, interesting. Linux is hitting assertion "info.mPendingCount > 0" with the patch. I wonder why the behavior is different on Windows.
Comment 3•6 years ago
|
||
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
Assignee | ||
Comment 4•6 years ago
|
||
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)
Updated•6 years ago
|
Blocks: stage-wr-nightly
Priority: -- → P2
Assignee | ||
Comment 5•6 years ago
|
||
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)
Assignee | ||
Comment 6•6 years ago
|
||
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)
Comment 7•6 years ago
|
||
(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.
Updated•6 years ago
|
Attachment #8949283 -
Flags: review?(nical.bugzilla) → review+
Comment 8•6 years ago
|
||
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.
Assignee | ||
Comment 9•6 years ago
|
||
The same patch but without the path checking (moving to bug 1433932).
Attachment #8949283 -
Attachment is obsolete: true
Attachment #8949360 -
Flags: review?(bugmail)
Comment 10•6 years ago
|
||
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+
Comment 11•6 years ago
|
||
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2c590c7dac39 Fixed render frame notification in wake_up(). r=nical
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2c590c7dac39
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•