1355720 - Check if ImageContainer is updated in ImageClientSingle::UpdateImage()

Status: RESOLVED INVALID

(Core :: Graphics: WebRender, defect)

Description

[Sotaro Ikeda [:sotaro]]

ImageClientSingle::UpdateImage() Does not check if ImageContainer is updated. 

WebRenderDisplayItemLayer::SendImageContainer() seems to permit a change of ImageContainer. But the UpdateImage() only compare GenerationCounter values.

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/wr/WebRenderDisplayItemLayer.cpp#40

Comment 1

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #0)
> ImageClientSingle::UpdateImage() Does not check if ImageContainer is updated. 

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/ImageClient.cpp#173

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Check if ImageContainer is updated in ImageClientSingle::UpdateImage()

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Check if ImageContainer is updated in ImageClientSingle::UpdateImage()

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Check if ImageContainer is updated in ImageClientSingle::UpdateImage()

Comment 5

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6862e9eb4e2789d530ebbb8cf45f53608bed87b

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8857358 [details] [diff] [review]
patch - Check if ImageContainer is updated in ImageClientSingle::UpdateImage()

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

::: gfx/layers/client/ImageClient.cpp
@@ +177,5 @@
>    uint32_t generationCounter;
>    aContainer->GetCurrentImages(&images, &generationCounter);
>  
> +  auto container = (uintptr_t)aContainer;
> +  if (mLastImageContainer == container &&

Since we're not holding a strong reference, isn't it possible that the ImageContainer got deleted and another one got allocated at the same memory address?

Can we just use a serial number on the ImageContainer and make sure we never reuse those (we do this for Image already).

Comment 7

[Sotaro Ikeda [:sotaro]]

Thanks for review. Yea, it seems safer to use a serial number on the ImageContainer.

WebRenderDisplayItemLayer hold strong reference of ImageContainer, but it might not work in another use cases.

Comment 8

[Sotaro Ikeda [:sotaro]]

Ah, generation counter is enough, since it is updated by using "Atomic<uint32_t> ImageContainer::sGenerationCounter"

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ImageContainer.cpp#222