Rendering error after scroll

VERIFIED FIXED in Firefox 61

Status

()

defect
P1
normal
VERIFIED FIXED
a year ago
9 months ago

People

(Reporter: alice0775, Assigned: mconley)

Tracking

({nightly-community, regression})

61 Branch
mozilla61
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60 unaffected, firefox61+ verified)

Details

(Whiteboard: [fxperf:p1], )

Attachments

(8 attachments)

Reporter

Description

a year ago
Posted file table.html
Build ID   : 20180319100039
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

It happens from time to time.
When Scroll down or up, the new visible area would not be drawn.
Mouse hover over the area do NOT help. 
Mouse clicking on the page will paint correctly.

Once happened the problem, it is somewhat easier to reproduce on the problematic tab.



Reproducible: It happens from time to time

Steps To Reproduce(not 100%): 

1. Bookmark the following page in the folder
   about:home
   the attached page
   the attached page
   the attached page
   the attached page
   the attached page
   the attached page
2. Restart browser and maximize
3. Middle click on the folder
   Now, you can see
   [tab1] about:home
   [tab2] about:home
   [tab3] the attached page
   [tab4] the attached page
   [tab5] the attached page
   [tab6] the attached page
   [tab7] the attached page
   [tab8] the attached page
4. For each tab, scroll up / down with mouse wheel or autoscroll

5. Close a tab and try Step4
6. Undo close and try Step4




I think it is difficult to find a regression window...



[NOTE]
* Setting layout.display-list.retain = false does NOT fix.
* Disabled HWA also does NOT fix.
Reporter

Comment 1

a year ago
Posted file about:support
Reporter

Comment 2

a year ago
* Setting layout.css.servo.enabled = false does NOT fix.
Reporter

Comment 3

a year ago
* Setting layers.omtp.enabled = false does NOT fix.
Reporter

Comment 4

a year ago
I had tested with str comment#0 for several hours. 
I cannot reproduce the problem If setting browser.tabs.remote.warmup.enabled = false.

So, I think Bug 1423220 triggeres this problem.
Reporter

Comment 5

a year ago
Posted image screenshot
Reporter

Updated

a year ago
Component: General → Layout: View Rendering

Comment 6

a year ago
I'm also experiencing the same issue lately. Not sure about if it's related to the tab warming feature, but it started to happen around the time it got into Nightly.
Reporter

Comment 7

a year ago
(In reply to Alice0775 White from comment #0)

In addition to comment#0, when mouse hover over scroll bar then the area is re-painted correctly.
Reporter

Comment 8

a year ago
Posted image screenshot2
This site is also affected.
https://hg.mozilla.org/mozilla-central/help
Reporter

Updated

a year ago
Summary: The newly displayed area after Scroll is not drawn → Rendering error after scroll
Mike, can you please take a look at this when you get a chance?
Flags: needinfo?(mconley)
Looking...
Assignee: nobody → mconley
Blocks: 1434651
Flags: needinfo?(mconley)
Reporter

Comment 11

a year ago
More reliable STR:

1. Bookmark the following page in the folder
   about:home
   https://hg.mozilla.org/mozilla-central/help
   https://hg.mozilla.org/mozilla-central/help
   https://hg.mozilla.org/mozilla-central/help
   https://hg.mozilla.org/mozilla-central/help
   https://hg.mozilla.org/mozilla-central/help
   https://hg.mozilla.org/mozilla-central/help
2. Restart browser and maximize
3. Middle click on the folder
   Now, you can see
   [tab1] about:home
   [tab2] about:home
   [tab3] https://hg.mozilla.org/mozilla-central/help
   [tab4] https://hg.mozilla.org/mozilla-central/help
   [tab5] https://hg.mozilla.org/mozilla-central/help
   [tab6] https://hg.mozilla.org/mozilla-central/help
   [tab7] https://hg.mozilla.org/mozilla-central/help
   [tab8] https://hg.mozilla.org/mozilla-central/help
4. Select [tab3], scroll to bottom

5. Close next tab(4th tab) with middle mouse click on tab
6. Scroll [tab3] to top with mouse wheel

if not reproduced
7. Scroll [tab3] to bottom with mouse wheel

if not reproduced
8. Repeat from step 5

Comment 12

a year ago
"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:61.0) Gecko/20100101 Firefox/61.0"

some problem
Unfortunately, the STR from comment don't seem to work with me. I suspect this is a race of some kind, so it's probably very machine-specific.

My current theory is that we're not unsuppressing displayport suppression soon enough and this is confusing things. I have a speculative patch that I'd like some folks to try coming up.
Here's a Windows try build with my speculative patch. Alice0775 White, since you're able to reproduce this semi-reliably, are you able to test these?

32-bit: https://queue.taskcluster.net/v1/task/Fi8oDs6xQoW5KiGQLSo-Fg/runs/0/artifacts/public/build/target.zip
64-bit: https://queue.taskcluster.net/v1/task/NQzCIwMfQqebXne0pT0KRw/runs/0/artifacts/public/build/target.zip
Flags: needinfo?(alice0775)
Reporter

Comment 16

a year ago
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #15)
> Here's a Windows try build with my speculative patch. Alice0775 White, since
> you're able to reproduce this semi-reliably, are you able to test these?
> 
> 32-bit:
> https://queue.taskcluster.net/v1/task/Fi8oDs6xQoW5KiGQLSo-Fg/runs/0/
> artifacts/public/build/target.zip
> 64-bit:
> https://queue.taskcluster.net/v1/task/NQzCIwMfQqebXne0pT0KRw/runs/0/
> artifacts/public/build/target.zip


The try build seems to fix this problem.

* I can reproduce the problem relatively easily on current Nightly61.0a1 64bit win10 with STR comment #11. After launching the Nightly, I was able to reproduce the problem of the first trial loop.

* On the other hand, I tested dozens of times, but I never reproduced the problem with the try build.

So, I can conclude that the try build fix this problem.
Flags: needinfo?(alice0775) → needinfo?(mconley)
Status: NEW → ASSIGNED
Has Regression Range: --- → yes
Has STR: --- → yes
OS: Windows 10 → Windows
QA Contact: Virtual
Hardware: x86_64 → All
Version: Trunk → 61 Branch

Comment 18

a year ago
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #15)
> Here's a Windows try build with my speculative patch. Alice0775 White, since
> you're able to reproduce this semi-reliably, are you able to test these?
> 
> 32-bit:
> https://queue.taskcluster.net/v1/task/Fi8oDs6xQoW5KiGQLSo-Fg/runs/0/
> artifacts/public/build/target.zip
> 64-bit:
> https://queue.taskcluster.net/v1/task/NQzCIwMfQqebXne0pT0KRw/runs/0/
> artifacts/public/build/target.zip

Virtual's STR in comment 11 seemed to always reproduce here, but now, using your build, I can't reproduce it even if I try several times.
Okay great, thanks. I'll get this patch up for review once I get a green try build.
Flags: needinfo?(mconley)
Comment hidden (mozreview-request)

Comment 23

a year ago
mozreview-review
Comment on attachment 8961928 [details]
Bug 1447193 - Remove all displayport suppression logic from AsyncTabSwitcher.

https://reviewboard.mozilla.org/r/230756/#review236804

::: browser/modules/AsyncTabSwitcher.jsm:287
(Diff revision 2)
>        this.maybeActivateDocShell(tab);
> +
> +      if (tabParent && this.activeSuppressDisplayport.has(tabParent)) {
> +        tabParent.suppressDisplayport(false);
> +        this.activeSuppressDisplayport.delete(tabParent);
> +      }

This looks good to me, but it has me wondering whether we're turning suppressDisplayport off too soon for warmed tabs that haven't been requested yet. Do we have any need to turn it on if we're not sure we're intending to display the given tab?
Attachment #8961928 - Flags: review?(dothayer) → review+
See Also: → 1448866
Priority: -- → P1
Assignee

Comment 24

a year ago
mozreview-review-reply
Comment on attachment 8961928 [details]
Bug 1447193 - Remove all displayport suppression logic from AsyncTabSwitcher.

https://reviewboard.mozilla.org/r/230756/#review236804

> This looks good to me, but it has me wondering whether we're turning suppressDisplayport off too soon for warmed tabs that haven't been requested yet. Do we have any need to turn it on if we're not sure we're intending to display the given tab?

Yeah, that's a good point. It also means that we could unsuppress the displayport in handling STATE_LOADED, and accidentally _re_-suppress it when requesting the tab. I'll come up with something else.
Comment hidden (mozreview-request)

Comment 26

a year ago
mozreview-review
Comment on attachment 8961928 [details]
Bug 1447193 - Remove all displayport suppression logic from AsyncTabSwitcher.

https://reviewboard.mozilla.org/r/230756/#review237576

Looks good to me!
Assignee

Comment 27

a year ago
mozreview-review-reply
Comment on attachment 8961928 [details]
Bug 1447193 - Remove all displayport suppression logic from AsyncTabSwitcher.

https://reviewboard.mozilla.org/r/230756/#review237576

Great, thanks!

Comment 28

a year ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de19108477c7
Unsuppress display ports immediately after the tab switch has completed. r=dthayer
Reporter

Comment 29

a year ago
(In reply to Pulsebot from comment #28)
> Pushed by mconley@mozilla.com:
> https://hg.mozilla.org/integration/autoland/rev/de19108477c7
> Unsuppress display ports immediately after the tab switch has completed.
> r=dthayer

The autoland build does not fix the problem.... :(
I can still reproduce the problem with STR of comment#11.

BuildID 20180328164502
SourceRepository=https://hg.mozilla.org/integration/autoland
SourceStamp=de19108477c77bbc07575825ecdf8fbe4609fce3
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Hey Alice, since you're able to reproduce this, I'm hoping you can help me debug it a bit more.

Can you please set apz.minimap.enabled to true, and then do a screen recording demonstrating the problem?
Flags: needinfo?(mconley) → needinfo?(alice0775)
Talked with kats for a bit and got myself some reliable STR.

0) Set dom.ipc.processCount to 1 and browser.tabs.remote.warmup.enabled and restart the browser
1) Open https://bug1447193.bmoattachments.org/attachment.cgi?id=8960415 in 3 or 4 tabs.
2) Shrink the window so that you have to scroll the page a bit. My window ended up being 450x750.
3) Select the first tab
4) Hover your mouse over the 2nd tab without clicking on it, to warm it up
5) Scroll the first tab up and down

This might take a few tries.
Flags: needinfo?(alice0775)

Comment 33

a year ago
Posted image Screencast
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #32)
> 0) Set dom.ipc.processCount to 1 and browser.tabs.remote.warmup.enabled and

FWIW, on my Nightly setup dom.ipc.processCount is set to 4.
See attached screencast.
Reporter

Comment 34

a year ago
Posted video screencast
set apz.minimap.enabled to true, capture with CamStudio. STR is comment#11
Attachment #8961928 - Flags: review+
Comment hidden (mozreview-request)
So it turns out that TabChild::RenderLayers does the suppression for us:

https://searchfox.org/mozilla-central/source/dom/ipc/TabChild.cpp#2649,2659

How embarrassing.

I'm pretty sure this means that AsyncTabSwitcher can just not do this suppression stuff now.

I've got a try build cooking here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d0cfe2d8504da0d023a19ec75abfe3476b26a46

Mind taking one more look at this, dthayer?
Flags: needinfo?(dothayer)
Attachment #8961928 - Flags: review?(dothayer)
I have a new build for people to try:

https://queue.taskcluster.net/v1/task/eQMltQb2SQCKWjr5yoomjQ/runs/0/artifacts/public/build/target.zip

Alice0775 White and Itiel, does this build improve things?
Flags: needinfo?(itiel_yn8)
Flags: needinfo?(alice0775)
Whiteboard: [fxperf:p1]

Comment 38

a year ago
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #37)
> I have a new build for people to try:
> 
> https://queue.taskcluster.net/v1/task/eQMltQb2SQCKWjr5yoomjQ/runs/0/
> artifacts/public/build/target.zip
> 
> Alice0775 White and Itiel, does this build improve things?

Seems to be fixed in with this build, with and without apz.minimap.enabled toggled on (if still relevant).
I'm seeing a different scrolling issue, but it has been like this for few days and probably not related to tab warming. But that's a story for a different bug.
Flags: needinfo?(itiel_yn8)

Comment 39

a year ago
mozreview-review
Comment on attachment 8961928 [details]
Bug 1447193 - Remove all displayport suppression logic from AsyncTabSwitcher.

https://reviewboard.mozilla.org/r/230756/#review237736

::: browser/modules/AsyncTabSwitcher.jsm
(Diff revisions 3 - 4)
> -  suppressDisplayPortAndQueueUnload(tab, unloadTimeout) {
> +  queueUnload(unloadTimeout) {
> -    let browser = tab.linkedBrowser;
> -    let fl = browser.frameLoader;
> -
> -    if (fl && fl.tabParent && !this.activeSuppressDisplayport.has(fl.tabParent)) {
> -      fl.tabParent.suppressDisplayport(true);

So, I guess I still don't understand why the maybeFinishTabSwitch change didn't fix things. If I'm understanding correctly, removing this fixes the issue because if the tab was warmed already, then we've already send the Renderlayers message, so this will get there after and turn it off, whereas before we would always be overridden by the RenderLayers message. I think this is more correct regardless, but it would be nice to understand why the previous change didn't work.
Attachment #8961928 - Flags: review?(dothayer) → review+
Assignee

Comment 40

a year ago
mozreview-review-reply
Comment on attachment 8961928 [details]
Bug 1447193 - Remove all displayport suppression logic from AsyncTabSwitcher.

https://reviewboard.mozilla.org/r/230756/#review237736

> So, I guess I still don't understand why the maybeFinishTabSwitch change didn't fix things. If I'm understanding correctly, removing this fixes the issue because if the tab was warmed already, then we've already send the Renderlayers message, so this will get there after and turn it off, whereas before we would always be overridden by the RenderLayers message. I think this is more correct regardless, but it would be nice to understand why the previous change didn't work.

I think I figured out what was going on. Bug 1449756 describes the problem - basically, if you have tabs A, B, C and D running in the same content process, then suppressing displayport on any of them effectively suppresses display port on _all_ of them (since it's a process global state. :/ ).

That is one half of it. The other half is that repaint after unsuppression is only triggered on the _last_ tab that's had its displayport unsuppressed.

So, here's the scenario: Tabs A, B, C and D. All in the same content process. A is selected.

I hover my mouse over B and then C, but don't leave the tab. We suppress displayport on B and C, which means _A_ is also suppressed. Then I start a scroll and a repaint occurs on A while suppression is still happening. Then suppression is removed from B and C. C gets a repaint. A never gets a repaint, and depending on the content of the page, might never get a repaint.

Basically, displayport suppression is a giant footgun as its currently designed, and we should fix bug 1449756, but this removal of setting displayport suppressions on multiple tabs should help us avoid some of the badness in the meantime.
Assignee

Comment 41

a year ago
mozreview-review-reply
Comment on attachment 8961928 [details]
Bug 1447193 - Remove all displayport suppression logic from AsyncTabSwitcher.

https://reviewboard.mozilla.org/r/230756/#review237736

> I think I figured out what was going on. Bug 1449756 describes the problem - basically, if you have tabs A, B, C and D running in the same content process, then suppressing displayport on any of them effectively suppresses display port on _all_ of them (since it's a process global state. :/ ).
> 
> That is one half of it. The other half is that repaint after unsuppression is only triggered on the _last_ tab that's had its displayport unsuppressed.
> 
> So, here's the scenario: Tabs A, B, C and D. All in the same content process. A is selected.
> 
> I hover my mouse over B and then C, but don't leave the tab. We suppress displayport on B and C, which means _A_ is also suppressed. Then I start a scroll and a repaint occurs on A while suppression is still happening. Then suppression is removed from B and C. C gets a repaint. A never gets a repaint, and depending on the content of the page, might never get a repaint.
> 
> Basically, displayport suppression is a giant footgun as its currently designed, and we should fix bug 1449756, but this removal of setting displayport suppressions on multiple tabs should help us avoid some of the badness in the meantime.

Whoops, forgot to say:

That's why the original patch seemed to work (we unsuppressed as layers came in and where more likely to have unsuppressed everything before the selected tab needed to paint again), and why the new one didn't (if we were still suppressing other tabs while running maybeFinishTabSwitch, we're no better off than before the patch).

Comment 42

a year ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3613e0e4fe2
Remove all displayport suppression logic from AsyncTabSwitcher. r=dthayer
Reporter

Updated

a year ago
Flags: needinfo?(alice0775)
Reporter

Comment 43

a year ago
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #37)
> I have a new build for people to try:
> 
> https://queue.taskcluster.net/v1/task/eQMltQb2SQCKWjr5yoomjQ/runs/0/
> artifacts/public/build/target.zip
> 
> Alice0775 White and Itiel, does this build improve things?


yes, it fixed. thanks.
Flags: needinfo?(dothayer)
Backed out changeset e3613e0e4fe2 (bug 1447193) for failing on tests/mochitest/test_mousecapture.xhtml on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/autoland/rev/e8c0d1031096c9335c5326a46f6cd6dc5b99c426

Try push with failures (autoland push had bustage and tests didn't run): https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d0cfe2d8504da0d023a19ec75abfe3476b26a46

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=170945389&repo=try&lineNumber=17239

[task 2018-03-29T01:36:21.517Z] 01:36:21     INFO - TEST-START | toolkit/content/tests/mochitest/test_bug1407085.html
[task 2018-03-29T01:36:21.614Z] 01:36:21     INFO - GECKO(6835) | MEMORY STAT | vsize 1559MB | residentFast 116MB | heapAllocated 18MB
[task 2018-03-29T01:36:21.617Z] 01:36:21     INFO - TEST-OK | toolkit/content/tests/mochitest/test_bug1407085.html | took 102ms
[task 2018-03-29T01:36:21.638Z] 01:36:21     INFO - TEST-START | toolkit/content/tests/mochitest/test_mousecapture.xhtml
[task 2018-03-29T01:41:50.097Z] 01:41:50     INFO - TEST-INFO | started process screentopng
[task 2018-03-29T01:41:50.614Z] 01:41:50     INFO - TEST-INFO | screentopng: exit 0
[task 2018-03-29T01:41:50.616Z] 01:41:50     INFO - Buffered messages logged at 01:36:21
[task 2018-03-29T01:41:50.616Z] 01:41:50     INFO - must wait for load
[task 2018-03-29T01:41:50.617Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | splitter target for point 112,124 
[task 2018-03-29T01:41:50.618Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | splitter left box size (2) 
[task 2018-03-29T01:41:50.619Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | splitter target for point 137,147 
[task 2018-03-29T01:41:50.621Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | splitter left box size (25) 
[task 2018-03-29T01:41:50.622Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | splitter target for point 4137,4122 
[task 2018-03-29T01:41:50.624Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | splitter left box size (4000) 
[task 2018-03-29T01:41:50.625Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | splitter target for point 198,110 
[task 2018-03-29T01:41:50.626Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | splitter left box size (-12) 
[task 2018-03-29T01:41:50.626Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml |  target for point 2,2 
[task 2018-03-29T01:41:50.628Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | splitter left box size (0) 
[task 2018-03-29T01:41:50.628Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | custom target for point 12,129 
[task 2018-03-29T01:41:50.629Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | custom target for point 35,152 
[task 2018-03-29T01:41:50.630Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | custom target for point 4010,4127 
[task 2018-03-29T01:41:50.631Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | custom target for point -2,115 
[task 2018-03-29T01:41:50.632Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml |  target for point 2,2 
[task 2018-03-29T01:41:50.632Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | setCapture and releaseCapture mousemove event target  
[task 2018-03-29T01:41:50.633Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | setCapture and releaseCapture mousemove event fired 
[task 2018-03-29T01:41:50.634Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | setCapture fails on non mousedown mousemove event target  
[task 2018-03-29T01:41:50.635Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | setCapture fails on non mousedown mousemove event fired 
[task 2018-03-29T01:41:50.637Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | document.releaseCapture releases capture mousemove event target  
[task 2018-03-29T01:41:50.638Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | document.releaseCapture releases capture mousemove event fired 
[task 2018-03-29T01:41:50.639Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | element.releaseCapture releases capture mousemove event target  
[task 2018-03-29T01:41:50.640Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | element.releaseCapture releases capture mousemove event fired 
[task 2018-03-29T01:41:50.642Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | element.releaseCapture during mousemove before releaseCapture mousemove event target  
[task 2018-03-29T01:41:50.643Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | element.releaseCapture during mousemove before releaseCapture mousemove event fired 
[task 2018-03-29T01:41:50.644Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | element.releaseCapture during mousemove after releaseCapture mousemove event target  
[task 2018-03-29T01:41:50.645Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | element.releaseCapture during mousemove after releaseCapture mousemove event fired 
[task 2018-03-29T01:41:50.646Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | custom5spacer target for point 12,169 
[task 2018-03-29T01:41:50.647Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | custom5inner target for point 35,192 
[task 2018-03-29T01:41:50.648Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | custom5 target for point 4010,4167 
[task 2018-03-29T01:41:50.649Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | custom5 target for point -2,155 
[task 2018-03-29T01:41:50.650Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml |  target for point 2,2 
[task 2018-03-29T01:41:50.651Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | custom5 target for point 12,169 
[task 2018-03-29T01:41:50.652Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | custom5 target for point 35,192 
[task 2018-03-29T01:41:50.653Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | custom5 target for point 4010,4167 
[task 2018-03-29T01:41:50.654Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | custom5 target for point -2,155 
[task 2018-03-29T01:41:50.655Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml |  target for point 2,2 
[task 2018-03-29T01:41:50.656Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | setCapture only works on elements in documents mousemove event target  
[task 2018-03-29T01:41:50.657Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | setCapture only works on elements in documents mousemove event fired 
[task 2018-03-29T01:41:50.657Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | setCapture works on images mousemove event target  
[task 2018-03-29T01:41:50.658Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | setCapture works on images mousemove event fired 
[task 2018-03-29T01:41:50.659Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | setCapture on body retargets to root node mousemove event target  
[task 2018-03-29T01:41:50.660Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | setCapture on body retargets to root node mousemove event fired 
[task 2018-03-29T01:41:50.660Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml |  target for point 52,52 
[task 2018-03-29T01:41:50.661Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | frameset after drag (2), new width 49, expected 49 
[task 2018-03-29T01:41:50.662Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml |  target for point 75,75 
[task 2018-03-29T01:41:50.663Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | frameset after drag (25), new width 72, expected 72 
[task 2018-03-29T01:41:50.664Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml |  target for point 4050,4050 
[task 2018-03-29T01:41:50.665Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | frameset after drag (4000), new width 92, expected 92 
[task 2018-03-29T01:41:50.665Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml |  target for point 38,38 
[task 2018-03-29T01:41:50.666Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | frameset after drag (-12), new width 35, expected 35 
[task 2018-03-29T01:41:50.667Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | frameset after drag (0), new width 35, expected 35 
[task 2018-03-29T01:41:50.668Z] 01:41:50     INFO - TEST-PASS | toolkit/content/tests/mochitest/test_mousecapture.xhtml | scroll select 
[task 2018-03-29T01:41:50.669Z] 01:41:50     INFO - must wait for load
[task 2018-03-29T01:41:50.669Z] 01:41:50     INFO - must wait for focus
[task 2018-03-29T01:41:50.670Z] 01:41:50     INFO - Buffered messages finished
[task 2018-03-29T01:41:50.671Z] 01:41:50     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/mochitest/test_mousecapture.xhtml | Test timed out.
Flags: needinfo?(mconley)
Argh - somehow this breaks some headless-mode tests. :/

Thanks RaulGurzau - investigating...
Flags: needinfo?(mconley)
Comment hidden (mozreview-request)
Well that was an adventure. Shout out to rr for making debugging this possible.

Basically, the suppressDisplayport message was coming down before the focus adjustment was being made. What that means is that the TabChild::RecvSuppressDisplayport was running, and then shortly after, TabChild::RecvActivate was running to focus the content.

The thing is that RecvSuppressDisplayport uses GetPresShell()[1], which ultimately ensures that a PresShell gets created[2].

Without the (superfluous) RecvSuppressDisplayport, the PresShell doesn't get created for a while later - _after_ TabChild::RecvActivate is received. TabChild::RecvActivate tries to focus the top-level content window using nsFocusManager::Focus, and that ultimately fails out because the PresShell doesn't exist.

So my solution here is to ensure that a PresShell exists when initting the TabChild. I don't know if that's a good idea or not, but I've got it baking on try, and I'm doing a Talos comparison:

Patch on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=23c8bd5cdf4125bad1cb6c8ff2eb83d8588098e6
Baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bef76b359ed82cf30ea73bf1b5403eccb461bf2
Compare: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=8bef76b359ed82cf30ea73bf1b5403eccb461bf2&newProject=try&newRevision=23c8bd5cdf4125bad1cb6c8ff2eb83d8588098e6&framework=1

[1]: https://searchfox.org/mozilla-central/rev/028cd8342899357d80fafba2d528a0fd5819e316/dom/ipc/TabChild.cpp#1354
[2]: https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/docshell/base/nsDocShell.cpp#4179
Hey smaug, I know you're kinda swamped right now, so feel free to redirect this - but is ensuring the PresShell exists inside TabChild::Init as I'm proposing here ill-advised?
Flags: needinfo?(bugs)
Hmm, doesn't that slow down tab creation? I mean, it should be, at least in theory, possible to create a new tab and not have PresShell for the initial about:blank but just the first page to load.
I'd rather not slow down tab creation if possible.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (only webcomponents and event handling reviews, please) from comment #50)

If it doesn't happen there, it seems to happen as soon as frame scripts start loading and one of them access content (which according to my testing, happens right here:

https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/browser/base/content/content.js#45
https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/browser/modules/ContextMenu.jsm#233

The reason I put this in TabChild::Init was to avoid this kind of funkyness down the road, but I _could_ also put PresShell creation in RecvActivate, to specialize the solution for this case. Would that be preferable?
Flags: needinfo?(bugs)
I guess if we create it anyhow, then doesn't matter much when. Early might make things simpler.
Still, we should think if we could avoid that.  But I don't really object the change.
Flags: needinfo?(bugs)
Attachment #8963690 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] (only webcomponents and event handling reviews, please) from comment #52)
> I guess if we create it anyhow, then doesn't matter much when. Early might
> make things simpler.
> Still, we should think if we could avoid that.  But I don't really object
> the change.

Cool. Welp, let's give it a shot.
Attachment #8963690 - Flags: review?(bugs)
Blocks: 1449116
See Also: → 1449116
Reporter

Updated

a year ago
No longer blocks: 1449116
Reporter

Updated

a year ago
See Also: 1449116
Okay, I dug into the Talos regression a bit. Here's what's going on:

When the displayport suppression count reaches 0, the last thing to be suppressed gets a paint scheduled for it. Removing the displayport suppression stuff from the async tab switcher means that in RecvRenderLayers, we'll suppress the displayport, paint, unsuppress the displayport, and schedule another paint. This makes sense - we want to paint the outer areas around the displayport to avoid checkerboarding.

The problem is that the MozAfterPaint seems to only fire for the _second_ paint, and the MozAfterPaint event is what the tps test relies on. So the regression that tps is reporting is basically a lie.

The tps test should probably be modified to measure time to MozLayerTreeReady instead - that's more truthful, since MozLayerTreeReady is what the parent uses to know that layers are ready for a tab and we can flip the deck.

So what I suggest is that we move the ensure-PresShell stuff back into RecvActivate, and file a separate bug to tighten up the tps test.
See Also: → 1451460
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 58

a year ago
mozreview-review
Comment on attachment 8963690 [details]
Bug 1447193 - Ensure PresShell exists when activating a TabChild to ensure focus can be properly set early in TabChild lifetime.

https://reviewboard.mozilla.org/r/232584/#review239460
Attachment #8963690 - Flags: review?(bugs) → review+
Thanks for the fast review!

Hey igoldan, just a heads up that this bug is likely going to cause a supposed tps regression. That's a failing of the test, and not these patches. I'm going to try to correct the test in bug 1451460. Just ni'ing you to give you the heads up.
(cc'ing, rather)

Comment 61

a year ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d99564b1aa39
Remove all displayport suppression logic from AsyncTabSwitcher. r=dthayer
https://hg.mozilla.org/integration/autoland/rev/9bad112e0374
Ensure PresShell exists when activating a TabChild to ensure focus can be properly set early in TabChild lifetime. r=smaug
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #59)
> Thanks for the fast review!
> 
> Hey igoldan, just a heads up that this bug is likely going to cause a
> supposed tps regression. That's a failing of the test, and not these
> patches. I'm going to try to correct the test in bug 1451460. Just ni'ing
> you to give you the heads up.

Thank you for the heads up!

Comment 63

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d99564b1aa39
https://hg.mozilla.org/mozilla-central/rev/9bad112e0374
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Hey Alice0775 White, have you seen the issue in the latest Nightly yet? Or can we mark this VERIFIED?
Flags: needinfo?(alice0775)
Reporter

Comment 65

a year ago
Build ID 	20180405104009
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Built from https://hg.mozilla.org/mozilla-central/rev/1b258f938525fda65ef80ffa0408bc665d5d8948

I can no longer reproduce the problem.
I will mark this as "verified fixed".
Status: RESOLVED → VERIFIED
Flags: needinfo?(alice0775)
Component: Layout: View Rendering → Layout: Web Painting
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.