Bug 183225 - imported/w3c/web-platform-tests/html/browsers/windows/browsing-context.html fails with async policy delegates
Summary: imported/w3c/web-platform-tests/html/browsers/windows/browsing-context.html f...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 184209
Blocks: 180568
  Show dependency treegraph
 
Reported: 2018-02-28 15:13 PST by Chris Dumez
Modified: 2018-04-18 09:49 PDT (History)
7 users (show)

See Also:


Attachments
WIP Patch (4.43 KB, patch)
2018-02-28 15:59 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.55 KB, patch)
2018-02-28 16:04 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.80 KB, patch)
2018-02-28 16:31 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-02-28 15:13:27 PST
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context.html fails with async policy delegates.
Comment 1 Radar WebKit Bug Importer 2018-02-28 15:14:08 PST
<rdar://problem/38003828>
Comment 2 Chris Dumez 2018-02-28 15:59:56 PST
Created attachment 334780 [details]
WIP Patch
Comment 3 Chris Dumez 2018-02-28 16:04:35 PST
Created attachment 334782 [details]
Patch
Comment 4 Chris Dumez 2018-02-28 16:31:22 PST
Created attachment 334783 [details]
Patch
Comment 5 Alex Christensen 2018-03-01 08:36:35 PST
Comment on attachment 334783 [details]
Patch

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

> LayoutTests/fast/loader/iframe-src-invalid-url-expected.txt:-3
> - - decidePolicyForNavigationAction 

I agree with the idea that we need to continue synchronously in the WebProcess.  We might need to do a decidePolicyForNavigationAction API call even though we've already continued for API compatibility.
Comment 6 Chris Dumez 2018-03-01 08:49:44 PST
(In reply to Alex Christensen from comment #5)
> Comment on attachment 334783 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334783&action=review
> 
> > LayoutTests/fast/loader/iframe-src-invalid-url-expected.txt:-3
> > - - decidePolicyForNavigationAction 
> 
> I agree with the idea that we need to continue synchronously in the
> WebProcess.  We might need to do a decidePolicyForNavigationAction API call
> even though we've already continued for API compatibility.

Can we try this and fall back to doing the IPC if we realize it is really needed?
I do not fully understand why we want to make the IPC call for about:blank.

And we we do the IPC, we have to keep the IPC sync and we'll ignore the client's decision if they respond asynchronously. I'd rather we do not ask the client if we're not always going to obey their decision in all cases.
Comment 7 Alex Christensen 2018-03-01 09:01:53 PST
Comment on attachment 334783 [details]
Patch

Clients might use this delegate callback to update state about what iframe is showing what URL, not to actually make a decision.  If you're willing to risk breaking such apps, and if you'll keep an eye out for such regressions, I'll r+ this change.
Comment 8 Chris Dumez 2018-03-01 09:04:00 PST
(In reply to Alex Christensen from comment #7)
> Comment on attachment 334783 [details]
> Patch
> 
> Clients might use this delegate callback to update state about what iframe
> is showing what URL, not to actually make a decision.  If you're willing to
> risk breaking such apps, and if you'll keep an eye out for such regressions,
> I'll r+ this change.

Yes, let's try. We know what to do as a fallback it is does cause breakage. For now, I'd rather try the more aggressive change.
Comment 9 Chris Dumez 2018-03-01 09:05:29 PST
Comment on attachment 334783 [details]
Patch

Clearing flags on attachment: 334783

Committed r229133: <https://trac.webkit.org/changeset/229133>
Comment 10 Chris Dumez 2018-03-01 09:05:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Chris Dumez 2018-03-01 10:44:07 PST
Follow-up API test fix in:
https://trac.webkit.org/r229136