Closed Bug 1382948 Opened 7 years ago Closed 7 years ago

Fix WebRenderBridgeParent::FlushTransactionIdsForEpoch()

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 4 obsolete files)

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: nobody → sotaro.ikeda.g
Blocks: 1377619
Attachment #8888687 - Flags: review?(nical.bugzilla)
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?
Attachment #8888687 - Flags: review?(nical.bugzilla)
(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.
Attachment #8888687 - Attachment is obsolete: true
Attachment #8889248 - Attachment is obsolete: true
(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.
(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.
Attachment #8889249 - Attachment is obsolete: true
Attachment #8889260 - Flags: review?(nical.bugzilla)
Attachment #8889260 - Flags: review?(nical.bugzilla)
Attachment #8889268 - Flags: review?(nical.bugzilla)
Attachment #8889268 - Flags: review?(nical.bugzilla) → review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3361a87e46
Fix WebRenderBridgeParent::FlushTransactionIdsForEpoch() r=nical
https://hg.mozilla.org/mozilla-central/rev/7c3361a87e46
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.