Bug 184206 – REGRESSION (r229828): Facebook login popup is blank

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Facebook login popup is blank on bestbuy.com since r229828.

[Chris Dumez]

<rdar://problem/39057006>

[Chris Dumez]

Created attachment 336906 [details]
WIP Patch

[Chris Dumez]

Created attachment 336908 [details]
Patch

[Wenson Hsieh]

Comment on attachment 336908 [details]
Patch

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

This seems reasonable to me given your analysis, but it would be great if someone familiar with loading code could take a look too.

r=mews

> Source/WebCore/ChangeLog:15
> +        FrameLoader in one in DocumentLoader for redirects. The calls sites in

Nit - …FrameLoader and one in DocumentLoader…

> Source/WebKit/ChangeLog:11
> +        pending. This assertion wwas being hit by several of our redirection

Nit - wwas => was

[Chris Dumez]

Created attachment 336913 [details]
Patch

[Chris Dumez]

Created attachment 336914 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 336914 [details]:

imported/w3c/web-platform-tests/dom/traversal/TreeWalker-currentNode.html bug 184208 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 336914 [details]
Patch

Clearing flags on attachment: 336914

Committed r230128: <https://trac.webkit.org/changeset/230128>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

Comment on attachment 336914 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Since r229828, we freeze the layer tree during the navigation policy check.

This seems like a bad regression in behavior of the WebKit API. You can see it with the soon-to-be-attached test program. When the navigation prompt is up, the counter stops updating and resizing the window doesn’t resize the web content. Why did we make this change?

Created attachment 336919 [details]
Test program

(In reply to mitz from comment #10)
> Comment on attachment 336914 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336914&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Since r229828, we freeze the layer tree during the navigation policy check.
> 
> This seems like a bad regression in behavior of the WebKit API. You can see
> it with the soon-to-be-attached test program. When the navigation prompt is
> up, the counter stops updating and resizing the window doesn’t resize the
> web content. Why did we make this change?

I guess I need to see if <https://trac.webkit.org/r230128> fixed the regression, and if not, attach my test program to a separate bug.

[Wenson Hsieh]

(In reply to mitz from comment #12)
> (In reply to mitz from comment #10)
> > Comment on attachment 336914 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=336914&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        Since r229828, we freeze the layer tree during the navigation policy check.
> > 
> > This seems like a bad regression in behavior of the WebKit API. You can see
> > it with the soon-to-be-attached test program. When the navigation prompt is
> > up, the counter stops updating and resizing the window doesn’t resize the
> > web content.

What is this a regression from? This seems to match behavior in Safari 11.0 and 11.1, with synchronous policy decisions.

(In reply to Wenson Hsieh from comment #13)
> (In reply to mitz from comment #12)
> > (In reply to mitz from comment #10)
> > > Comment on attachment 336914 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=336914&action=review
> > > 
> > > > Source/WebCore/ChangeLog:9
> > > > +        Since r229828, we freeze the layer tree during the navigation policy check.
> > > 
> > > This seems like a bad regression in behavior of the WebKit API. You can see
> > > it with the soon-to-be-attached test program. When the navigation prompt is
> > > up, the counter stops updating and resizing the window doesn’t resize the
> > > web content.
> 
> What is this a regression from? This seems to match behavior in Safari 11.0
> and 11.1, with synchronous policy decisions.

The test program isn’t calling the decision handler synchronously. Try it (or better yet, try the version I just uploaded to bug 184209) in macOS 10.13.4 with the built-in WebKit and with TOT WebKit and see the difference in behavior when clicking the WebKit link.

(In reply to mitz from comment #14)
> (In reply to Wenson Hsieh from comment #13)
> > (In reply to mitz from comment #12)
> > > (In reply to mitz from comment #10)
> > > > Comment on attachment 336914 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=336914&action=review
> > > > 
> > > > > Source/WebCore/ChangeLog:9
> > > > > +        Since r229828, we freeze the layer tree during the navigation policy check.
> > > > 
> > > > This seems like a bad regression in behavior of the WebKit API. You can see
> > > > it with the soon-to-be-attached test program. When the navigation prompt is
> > > > up, the counter stops updating and resizing the window doesn’t resize the
> > > > web content.
> > 
> > What is this a regression from? This seems to match behavior in Safari 11.0
> > and 11.1, with synchronous policy decisions.
> 
> The test program isn’t calling the decision handler synchronously. Try it
> (or better yet, try the version I just uploaded to bug 184209) in macOS
> 10.13.4 with the built-in WebKit and with TOT WebKit and see the difference
> in behavior when clicking the WebKit link.

Although, for what it’s worth, I am seeing the regression in Safari as well: with Safari 11.1 with built-in WebKit, if I navigate to <https://bugs.webkit.org/query.cgi>, choose the Advanced tab, type “malloc” in the search field, hit Return, wait for the results page to load, then hit Command-R, then I resize the window while the “Are you sure you want to send a form again?” sheet is up, then the web content relayouts as I resize, which is expected. If I do the same thing in the same Safari but using TOT WebKit, then the web content doesn’t resize while the sheet is up.

[Wenson Hsieh]

(In reply to mitz from comment #14)
> (In reply to Wenson Hsieh from comment #13)
> > (In reply to mitz from comment #12)
> > > (In reply to mitz from comment #10)
> > > > Comment on attachment 336914 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=336914&action=review
> > > > 
> > > > > Source/WebCore/ChangeLog:9
> > > > > +        Since r229828, we freeze the layer tree during the navigation policy check.
> > > > 
> > > > This seems like a bad regression in behavior of the WebKit API. You can see
> > > > it with the soon-to-be-attached test program. When the navigation prompt is
> > > > up, the counter stops updating and resizing the window doesn’t resize the
> > > > web content.
> > 
> > What is this a regression from? This seems to match behavior in Safari 11.0
> > and 11.1, with synchronous policy decisions.
> 
> The test program isn’t calling the decision handler synchronously. Try it
> (or better yet, try the version I just uploaded to bug 184209) in macOS
> 10.13.4 with the built-in WebKit and with TOT WebKit and see the difference
> in behavior when clicking the WebKit link.

By "synchronous policy decisions", I mean blocking the web process on policy decisions in the UI process, not the UI process itself being blocked during policy decision. I was under the impression Chris had fixed synchronous policy decisions in <https://bugs.webkit.org/show_bug.cgi?id=180568>...

(In reply to Wenson Hsieh from comment #16)
> By "synchronous policy decisions", I mean blocking the web process on policy
> decisions in the UI process, not the UI process itself being blocked during
> policy decision. 

Blocking the Web Content process on policy decisions was a short-lived regression that was fixed a long time ago (see bug 177590).

> I was under the impression Chris had fixed synchronous
> policy decisions in <https://bugs.webkit.org/show_bug.cgi?id=180568>...

(In reply to mitz from comment #12)
> I guess I need to see if <https://trac.webkit.org/r230128> fixed the
> regression, and if not, attach my test program to a separate bug.

Not fixed, so I filed bug 184210.

[Chris Dumez]

(In reply to mitz from comment #17)
> (In reply to Wenson Hsieh from comment #16)
> > By "synchronous policy decisions", I mean blocking the web process on policy
> > decisions in the UI process, not the UI process itself being blocked during
> > policy decision. 
> 
> Blocking the Web Content process on policy decisions was a short-lived
> regression that was fixed a long time ago (see bug 177590).
> 
> > I was under the impression Chris had fixed synchronous
> > policy decisions in <https://bugs.webkit.org/show_bug.cgi?id=180568>...

The Policy IPC was synchronous and most clients responded synchronously. Responding asynchronously would use a different untested code path riddled with bugs.

With recent changes the IPC is async and even if the client responds Synchronously, it will be asynchronous from Webcore’s point of view. The sync code path is gone and everybody now uses the asynchronous code path now. As I said, there were a lot of bugs with this code path so we landed a lot of fixes before forcing everyone into the async code path.

(In reply to Chris Dumez from comment #19)
> (In reply to mitz from comment #17)
> > (In reply to Wenson Hsieh from comment #16)
> > > By "synchronous policy decisions", I mean blocking the web process on policy
> > > decisions in the UI process, not the UI process itself being blocked during
> > > policy decision. 
> > 
> > Blocking the Web Content process on policy decisions was a short-lived
> > regression that was fixed a long time ago (see bug 177590).
> > 
> > > I was under the impression Chris had fixed synchronous
> > > policy decisions in <https://bugs.webkit.org/show_bug.cgi?id=180568>...
> 
> The Policy IPC was synchronous and most clients responded synchronously.
> Responding asynchronously would use a different untested code path riddled
> with bugs.

This doesn’t sound accurate, because Safari has been responding asynchronously in some cases for over a decade on both macOS and iOS.

> 
> With recent changes the IPC is async and even if the client responds
> Synchronously, it will be asynchronous from Webcore’s point of view. The
> sync code path is gone and everybody now uses the asynchronous code path
> now. As I said, there were a lot of bugs with this code path so we landed a
> lot of fixes before forcing everyone into the async code path.

So it sounds like, in response to my question in bug 180568 comment 149, the behavior of the API as observed by any clients should not be changing. It may be helpful to use a test program like the one I’ve attached here, or maybe an automated version of that test, while making changes to ensure that the behavior really doesn’t change.

Thanks!

[Chris Dumez]

(In reply to mitz from comment #20)
> (In reply to Chris Dumez from comment #19)
> > (In reply to mitz from comment #17)
> > > (In reply to Wenson Hsieh from comment #16)
> > > > By "synchronous policy decisions", I mean blocking the web process on policy
> > > > decisions in the UI process, not the UI process itself being blocked during
> > > > policy decision. 
> > > 
> > > Blocking the Web Content process on policy decisions was a short-lived
> > > regression that was fixed a long time ago (see bug 177590).
> > > 
> > > > I was under the impression Chris had fixed synchronous
> > > > policy decisions in <https://bugs.webkit.org/show_bug.cgi?id=180568>...
> > 
> > The Policy IPC was synchronous and most clients responded synchronously.
> > Responding asynchronously would use a different untested code path riddled
> > with bugs.
> 
> This doesn’t sound accurate, because Safari has been responding
> asynchronously in some cases for over a decade on both macOS and iOS.

Well, when we made WebKitTestRunner respond asynchronously to the policy delegates, hundreds of layout tests started failing, most of them due to legit WebCore bugs. We spent a lot of time fixing those bugs in the last month.

> > 
> > With recent changes the IPC is async and even if the client responds
> > Synchronously, it will be asynchronous from Webcore’s point of view. The
> > sync code path is gone and everybody now uses the asynchronous code path
> > now. As I said, there were a lot of bugs with this code path so we landed a
> > lot of fixes before forcing everyone into the async code path.
> 
> So it sounds like, in response to my question in bug 180568 comment 149, the
> behavior of the API as observed by any clients should not be changing. It
> may be helpful to use a test program like the one I’ve attached here, or
> maybe an automated version of that test, while making changes to ensure that
> the behavior really doesn’t change.
> 
> Thanks!