Closed
Bug 1382948
Opened 7 years ago
Closed 7 years ago
Fix WebRenderBridgeParent::FlushTransactionIdsForEpoch()
Categories
(Core :: Graphics: WebRender, enhancement)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: sotaro, Assigned: sotaro)
References
Details
Attachments
(1 file, 4 obsolete files)
6.18 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
The FlushTransactionIdsForEpoch() has a problem that needs to be addressed. It could not handle well a situation that a completed epoch is smaller than a pending epoc.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60263cdd516f19759b3c031f04c150fbbcc906bf
Assignee | ||
Updated•7 years ago
|
Attachment #8888687 -
Flags: review?(nical.bugzilla)
Comment 3•7 years ago
|
||
Comment on attachment 8888687 [details] [diff] [review] patch - Fix WebRenderBridgeParent::FlushTransactionIdsForEpoch() Review of attachment 8888687 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/wr/WebRenderBridgeParent.cpp @@ +1168,5 @@ > WebRenderBridgeParent::FlushTransactionIdsForEpoch(const wr::Epoch& aEpoch) > { > uint64_t id = 0; > while (!mPendingTransactionIds.empty()) { > + int32_t diff = static_cast<int32_t>(wr::AsUint32(aEpoch) - wr::AsUint32(mPendingTransactionIds.front().mEpoch)); wr::AsUint32(aEpoch) - wr::AsUint32(mPendingTransactionIds.front().mEpoch) can underflow as an uint32 (so the result of the subtraction will be positive in all cases), and the cast to int32 could maybe wrap it back into the negative space, but could also not, because it is (I think) undefined behavior. I think that the correct thing to do would be to cast to int64 both epochs before subtracting. ::: gfx/webrender_bindings/WebRenderTypes.h @@ +43,5 @@ > } > > +// Whenever possible, use wr::Epoch instead of manipulating uint32_t. > +inline uint32_t AsUint32(const Epoch& aEpoch) { > + return static_cast<uint32_t>(aEpoch.mHandle); Why do we need to static_cast here, isn't mHandle already a uint32_t?
Assignee | ||
Updated•7 years ago
|
Attachment #8888687 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #3) > Comment on attachment 8888687 [details] [diff] [review] > patch - Fix WebRenderBridgeParent::FlushTransactionIdsForEpoch() > > Review of attachment 8888687 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/wr/WebRenderBridgeParent.cpp > @@ +1168,5 @@ > > WebRenderBridgeParent::FlushTransactionIdsForEpoch(const wr::Epoch& aEpoch) > > { > > uint64_t id = 0; > > while (!mPendingTransactionIds.empty()) { > > + int32_t diff = static_cast<int32_t>(wr::AsUint32(aEpoch) - wr::AsUint32(mPendingTransactionIds.front().mEpoch)); > > wr::AsUint32(aEpoch) - wr::AsUint32(mPendingTransactionIds.front().mEpoch) > can underflow as an uint32 (so the result of the subtraction will be > positive in all cases), and the cast to int32 could maybe wrap it back into > the negative space, but could also not, because it is (I think) undefined > behavior. Yea, it is undefined behaviour. > > I think that the correct thing to do would be to cast to int64 both epochs > before subtracting. > > ::: gfx/webrender_bindings/WebRenderTypes.h > @@ +43,5 @@ > > } > > > > +// Whenever possible, use wr::Epoch instead of manipulating uint32_t. > > +inline uint32_t AsUint32(const Epoch& aEpoch) { > > + return static_cast<uint32_t>(aEpoch.mHandle); > > Why do we need to static_cast here, isn't mHandle already a uint32_t? OK, I am going to use mHandle directly.
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8888687 -
Attachment is obsolete: true
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8889248 -
Attachment is obsolete: true
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #3) > I think that the correct thing to do would be to cast to int64 both epochs > before subtracting. Selected CheckedUint32 instead of int64.
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #7) > (In reply to Nicolas Silva [:nical] from comment #3) > > I think that the correct thing to do would be to cast to int64 both epochs > > before subtracting. > > Selected CheckedUint32 instead of int64. I changed my mind. int64 looks simpler.
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8889249 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8889260 -
Flags: review?(nical.bugzilla)
Assignee | ||
Updated•7 years ago
|
Attachment #8889260 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8889260 -
Attachment is obsolete: true
Assignee | ||
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7f8d266c0d3453af2ecfe9bcb2beca718c56c0e
Assignee | ||
Updated•7 years ago
|
Attachment #8889268 -
Flags: review?(nical.bugzilla)
Updated•7 years ago
|
Attachment #8889268 -
Flags: review?(nical.bugzilla) → review+
Comment 12•7 years ago
|
||
Pushed by sikeda@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3361a87e46 Fix WebRenderBridgeParent::FlushTransactionIdsForEpoch() r=nical
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7c3361a87e46
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•