1407480 - TalosError("timeout") on tabpaint

Status: RESOLVED FIXED

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

Description

[Vincent Liu[:vliu]]

It is still has this timeout error with recent try build. Please see [1] for detail.

[1]: https://treeherder.mozilla.org/logviewer.html#?job_id=135642600&repo=try&lineNumber=4283

Comment 1

[Vincent Liu[:vliu]]

The timeout happens because rect.width and rect.height became zero and caused a early return[1]. In this case, the async message TabPaint:Painted didn't called and finally timeout.

[1]: http://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/testing/talos/talos/tests/tabpaint/content/target.html#25

Comment 2

[Vincent Liu[:vliu]]

Attachment: 0001-Bug-1407480-Carry-valid-Rect-when-calling-NotifyInva.patch

Hi kats,
The attached patch intends to fix the timeout for tabpaint tolas test. Like the description in Comment, timeout happens because rect.height became zero.
The thing is zero width and height was passed when we called NotifyInvalidation() in [1] in layers-free mode. To solve it, I tried to get valid Rect from nsIFrame and passed into it. I'd sent this patch and it can pass the try. Please see [2].

[1]: http://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/layout/painting/nsDisplayList.cpp#2189
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ed76d6a38c87d649ba4743e719fb624c12a0883

Could you please have a review? Thanks

Comment 3

[Ethan Lin[:ethlin]]

Per discussion with Vincent, looks like the talos test checks if the invalidation region is empty before starting. So we should pass a non-empty rect. Originally we pass an empty rect is because we are not sure who uses the invalidation rect. For WR, I think the invalidation rect should be the whole frame.

Comment 4

[Ethan Lin[:ethlin]]

Comment on attachment 8917711 [details] [diff] [review]
0001-Bug-1407480-Carry-valid-Rect-when-calling-NotifyInva.patch

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

::: layout/painting/nsDisplayList.cpp
@@ +2186,5 @@
>      }
>  
>      if (presContext->RefreshDriver()->HasScheduleFlush()) {
> +      nsRect rect = frame->GetRect();
> +      presContext->NotifyInvalidation(layerManager->GetLastTransactionId(), rect);

I think you can pass 'frame->GetRect()' directly.

Comment 5

[Vincent Liu[:vliu]]

Attachment: 0001-Bug-1407480-Carry-valid-Rect-when-calling-NotifyInva.patch

(In reply to Ethan Lin[:ethlin] from comment #4)
> Comment on attachment 8917711 [details] [diff] [review]
> 0001-Bug-1407480-Carry-valid-Rect-when-calling-NotifyInva.patch
> 
> Review of attachment 8917711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/painting/nsDisplayList.cpp
> @@ +2186,5 @@
> >      }
> >  
> >      if (presContext->RefreshDriver()->HasScheduleFlush()) {
> > +      nsRect rect = frame->GetRect();
> > +      presContext->NotifyInvalidation(layerManager->GetLastTransactionId(), rect);
> 
> I think you can pass 'frame->GetRect()' directly.

Sure. it is more clear for the patch.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8917735 [details] [diff] [review]
0001-Bug-1407480-Carry-valid-Rect-when-calling-NotifyInva.patch

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

This seems reasonable to me. Thanks!

Comment 7

[Vincent Liu[:vliu]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fb435173ec0822f4561b9c757c7148e37a9b222&selectedJob=136509949

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ead77d51be9
Carry valid Rect when calling NotifyInvalidation() in layers-free mode. r=kats

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6ead77d51be9