Closed Bug 1447193 Opened 6 years ago Closed 6 years ago

Rendering error after scroll

Categories

(Core :: Web Painting, defect, P1)

Product:
Core
Component:
Web Painting
Version:
61 Branch
Platform:
All
Windows
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 + verified

People

(Reporter: alice0775, Assigned: mconley)

References

(
URL
)

Details

(Keywords: nightly-community, regression, Whiteboard: [fxperf:p1])

Attachments

(8 files)

table.html
24.68 KB, text/html
Details
about:support
14.31 KB, text/plain
Details
screenshot
95.10 KB, image/png
Details
screenshot2
40.37 KB, image/png
Details
Bug 1447193 - Remove all displayport suppression logic from AsyncTabSwitcher.
59 bytes, text/x-review-board-request
alexical
: review+
Details
Screencast
5.77 MB, image/gif
Details
screencast
497.08 KB, video/mp4
Details
Bug 1447193 - Ensure PresShell exists when activating a TabChild to ensure focus can be properly set early in TabChild lifetime.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Attached 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.
Attached file about:support — 

    
    

    
  
* Setting layout.css.servo.enabled = false does NOT fix.

    
    

    
  
* Setting layers.omtp.enabled = false does NOT fix.

    
    

    
  
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.
Blocks: 1423220
URL: http://www.osaka-kasen-portal.net/sui...

          status-firefox59:
          --- → unaffected

          status-firefox60:
          --- → unaffected

          status-firefox-esr52:
          --- → unaffected
Keywords: dupeme → regression

    
    

    
  

      
      
      
      

        Attached image
          
        screenshot
        — 
      

    


  

    
  
Component: General → Layout: View Rendering

    
    

    
  
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.

    
    

    
  
(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.

    
    

    
  

      
      
      
      

        Attached image
          
        screenshot2
        — 
      

    


  
This site is also affected.
https://hg.mozilla.org/mozilla-central/help

    
  
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?

          tracking-firefox61:
          --- → +
Flags: needinfo?(mconley)

    
    

    
  
Looking...
Assignee: nobody → mconley
Blocks: 1434651
Flags: needinfo?(mconley)

    
    

    
  
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

    
    

    
  
"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)

    
    

    
  
(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
Keywords: nightly-community
OS: Windows 10 → Windows
QA Contact: Virtual
Hardware: x86_64 → All
Version: Trunk → 61 Branch

    
    

    
  
(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 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

    
    

    
  
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 on attachment 8961928 [details]
Bug 1447193 - Remove all displayport suppression logic from AsyncTabSwitcher.

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

Looks good to me!

    
    

    
  
Comment on attachment 8961928 [details]
Bug 1447193 - Remove all displayport suppression logic from AsyncTabSwitcher.

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

Great, thanks!

    
    

    
  
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

    
    

    
  
(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)

    
    

    
  

      
      
      
      

        Attached 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.

    
    

    
  

      
      
      
      

        Attached video
          
        screencast
        — 
      

    


  
set apz.minimap.enabled to true, capture with CamStudio. STR is comment#11

    
  

        Attachment #8961928 -
        Flags: review+

    
    

    
      
  

    
    

    
  
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]

    
    

    
  
(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 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+

    
    

    
  
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.

    
    

    
  
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).

    
    

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

    
  
Flags: needinfo?(alice0775)

    
    

    
  
(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)

    
    

    
      
  

    
    

    
  
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

    
  
No longer blocks: 1449116

    
  
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 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)

    
    

    
  
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!

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/d99564b1aa39
https://hg.mozilla.org/mozilla-central/rev/9bad112e0374
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox61:
          affected → fixed
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)

    
    

    
  
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

          status-firefox61:
          fixed → verified
Flags: needinfo?(alice0775)
Component: Layout: View Rendering → Layout: Web Painting

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: