REGRESSION (WEBPROCESS_WINDOWSERVER_BLOCKING): requestAnimationFrame Stops Completing

[Daniel James]

Created attachment 353045 [details]
Minimal Reproducible Example

The gist is that since Safari 12, it's possible to "break" requestAnimationFrame for every page in a grouped process. This makes our application practically unusable in  Safari.

I have attached a minimal reproducible example. I have verified that Safari 11 / Chrome / etc.. does not have this problem, and that this problem still occurs in the latest Safari Technology Preview (68)

Instructions:
Open index.html
Notice an incrementing counter.
Click the link to take you to loop.html
Notice the incrementing counter begins after some delay.
Without going back to the first page, close loop.html, taking you back to index.html

Expected:
The counters should continually increment while viewing the pages.

Actual:
Both: The counter in no longer incrementing. 
Safari 12: Other pages spawned by this page appear to get 0 or 1 completions to requestAnimationFrame and no more. (Additionally, interacting with the dev tools open in this state tends to crash the process, but I can't reproduce this reliably.)
Preview 68: Opening a new page tends to fix the counter. 


Example website:
Opening a document at www.lucidchart.com will put Safari 12 into this state.

[Radar WebKit Bug Importer]

<rdar://problem/45556367>

[Daniel James]

This appears at least partially OS dependent. We could not reproduce this in Safari 12 on High Sierra.

[Chris Dumez]

This is related to Per's sandboxing of the WebProcess on Mojave to that the WebProcess no longer has access to the Window server.

Before https://trac.webkit.org/changeset/232879/webkit:
- rAF keeps incrementing in index.html when closing loop.html
but
- rAF fails to increment in loop.html

After https://trac.webkit.org/changeset/232879/webkit:
- rAF is no longer incrementing in index.html when closing loop.html
but
- rAF is incrementing in loop.html when opening it.

[Chris Dumez]

As a quick hack, I tried:
diff --git a/Source/WebKit/UIProcess/mac/WebPageProxyMac.mm b/Source/WebKit/UIProcess/mac/WebPageProxyMac.mm
index b55ce9e8f39..4c877511ca1 100644
--- a/Source/WebKit/UIProcess/mac/WebPageProxyMac.mm
+++ b/Source/WebKit/UIProcess/mac/WebPageProxyMac.mm
@@ -695,8 +695,8 @@ void WebPageProxy::stopDisplayLink(unsigned observerID)
         return;
     
     m_displayLink->removeObserver(observerID);
-    if (!m_displayLink->hasObservers())
-        m_displayLink = nullptr;
+    /*if (!m_displayLink->hasObservers())
+        m_displayLink = nullptr;*/
 }
 #endif


It seems to fix the problem. Sending over to Per since he wrote this code and I am not familiar with it.

[Per Arne Vollan]

Created attachment 354387 [details]
Patch

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #5)
> Created attachment 354387 [details]
> Patch

WIP.

[Per Arne Vollan]

Created attachment 354388 [details]
Patch

[EWS Watchlist]

Attachment 354388 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Created attachment 354677 [details]
Patch

[Dean Jackson]

Comment on attachment 354677 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354677&action=review

> Source/WebKit/ChangeLog:8
> +        Since the Web page currently ownes the display link on the UI process side, the display link will be

Typo: owns

[EWS Watchlist]

Comment on attachment 354677 [details]
Patch

Attachment 354677 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9974944

New failing tests:
fast/animation/request-animation-frame-in-two-pages.html

[EWS Watchlist]

Created attachment 354686 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Per Arne Vollan]

Created attachment 354819 [details]
Patch

[Per Arne Vollan]

Created attachment 354823 [details]
Patch

[Per Arne Vollan]

(In reply to Dean Jackson from comment #10)
> Comment on attachment 354677 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354677&action=review
> 
> > Source/WebKit/ChangeLog:8
> > +        Since the Web page currently ownes the display link on the UI process side, the display link will be
> 
> Typo: owns

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 354823 [details]
Patch

Clearing flags on attachment: 354823

Committed r238184: <https://trac.webkit.org/changeset/238184>

[Lucas Forschler]

Mass move bugs into the DOM component.