Bug 195126 - Universal links from Google search results pages don't open the app
Summary: Universal links from Google search results pages don't open the app
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-27 15:59 PST by Brady Eidson
Modified: 2019-03-01 10:34 PST (History)
10 users (show)

See Also:


Attachments
Patch (8.72 KB, patch)
2019-02-27 16:06 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (1.42 KB, patch)
2019-02-28 17:32 PST, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2019-02-27 15:59:33 PST
Universal links from Google search results pages don't open the app

They do some fancy iframe tricks.

We have a good relaxation to fix it.
Comment 1 Brady Eidson 2019-02-27 15:59:44 PST
<rdar://problem/46887179>
Comment 2 Brady Eidson 2019-02-27 16:06:38 PST
Created attachment 363150 [details]
Patch
Comment 3 Build Bot 2019-02-27 16:09:27 PST
Attachment 363150 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ShouldOpenExternalURLsInNewWindowActions.mm:257:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ShouldOpenExternalURLsInNewWindowActions.mm:261:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ShouldOpenExternalURLsInNewWindowActions.mm:263:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ShouldOpenExternalURLsInNewWindowActions.mm:265:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ShouldOpenExternalURLsInNewWindowActions.mm:273:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 5 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Geoffrey Garen 2019-02-27 16:23:53 PST
Comment on attachment 363150 [details]
Patch

r=me
Comment 5 WebKit Commit Bot 2019-02-27 16:50:36 PST
Comment on attachment 363150 [details]
Patch

Clearing flags on attachment: 363150

Committed r242181: <https://trac.webkit.org/changeset/242181>
Comment 6 WebKit Commit Bot 2019-02-27 16:50:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Geoffrey Garen 2019-02-28 13:27:01 PST
ASSERTION FAILED: result == (origin1.toString() == origin2.toString())
./page/SecurityOrigin.cpp(498) : bool WebCore::originsMatch(const WebCore::SecurityOrigin &, const WebCore::SecurityOrigin &)
1   0x368180a99 WTFCrash
2   0x3560ea8db WTFCrashWithInfo(int, char const*, char const*, int)
3   0x359069c05 WebCore::originsMatch(WebCore::SecurityOrigin const&, WebCore::SecurityOrigin const&)
4   0x358d706be WebCore::DocumentLoader::shouldOpenExternalURLsPolicyToPropagate() const
5   0x358db5666 WebCore::HistoryController::saveDocumentState()
Comment 9 Ryan Haddad 2019-02-28 16:20:20 PST
It appears that mac-debug EWS saw this before the change was landed, but it didn't get a chance to verify results:
https://webkit-queues.webkit.org/results/11309974

Since these are consistent crashes affecting EWS, we should roll this out if it cannot be fixed quickly.
Comment 10 Brady Eidson 2019-02-28 17:02:25 PST
I'm sure this'll be easy to resolve but I will not be able to do so today.
Comment 11 Brady Eidson 2019-02-28 17:03:56 PST
Actually let me see if I can in the next 10 minutes. Hang on.
Comment 12 Brady Eidson 2019-02-28 17:11:26 PST
Have to build for desktop from scratch. Will check on it when I get home and try to resolve this tonight.
Comment 13 Brady Eidson 2019-02-28 17:12:23 PST
(In reply to Ryan Haddad from comment #9)
> It appears that mac-debug EWS saw this before the change was landed, but it
> didn't get a chance to verify results:
> https://webkit-queues.webkit.org/results/11309974

I've been an advocate for a cq+ that waits for EWS clearance to actually land for quite awhile now, and I sometimes forget it doesn't already do it. =/
Comment 14 Brady Eidson 2019-02-28 17:30:41 PST
Bogus assertion.

They are both file:// origins, one with universal access and the other without.

The one without toStrings() to "null"
The one with toStrings() to "file"

Removing the ASSERT until somebody explains to me why it's important (it was added in https://trac.webkit.org/changeset/207871/webkit with no explanation as to why)
Comment 15 Brady Eidson 2019-02-28 17:32:46 PST
Reopening to attach new patch.
Comment 16 Brady Eidson 2019-02-28 17:32:47 PST
Created attachment 363281 [details]
Patch
Comment 17 WebKit Commit Bot 2019-02-28 18:10:28 PST
Comment on attachment 363281 [details]
Patch

Clearing flags on attachment: 363281

Committed r242249: <https://trac.webkit.org/changeset/242249>
Comment 18 WebKit Commit Bot 2019-02-28 18:10:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 youenn fablet 2019-02-28 19:34:18 PST
> Removing the ASSERT until somebody explains to me why it's important (it was
> added in https://trac.webkit.org/changeset/207871/webkit with no explanation
> as to why)

https://trac.webkit.org/changeset/207871/webkit is adding the helper routine based on a previous version which was doing string matching.
The ASSERT makes sure that both versions are equivalent.

String matching is useful as this is what origins end up being serialized to, for instance in HTTP headers for CORS.

In this particular case, universal access makes things like CORS a no-problem so this is fine to ignore.

I see value in tracking the differences between the two comparisons and making sure any difference is ok.
I would tend to beef up the ASSERT instead of removing it.
Comment 20 Geoffrey Garen 2019-02-28 20:19:09 PST
> I would tend to beef up the ASSERT instead of removing it.

What would you recommend here? Retain the ASSERT but exclude file:// origins with universal access?
Comment 21 Brady Eidson 2019-03-01 08:45:49 PST
(In reply to youenn fablet from comment #19)
> > Removing the ASSERT until somebody explains to me why it's important (it was
> > added in https://trac.webkit.org/changeset/207871/webkit with no explanation
> > as to why)
> 
> https://trac.webkit.org/changeset/207871/webkit is adding the helper routine
> based on a previous version which was doing string matching.
> The ASSERT makes sure that both versions are equivalent.

As is evidenced by the impact this patch had on the tests, the two versions are *not* equivalent.

If they are supposed to be equivalent then the original patch seems wrong.

> I see value in tracking the differences between the two comparisons and
> making sure any difference is ok.
> I would tend to beef up the ASSERT instead of removing it.

I'm fine with restoring an ASSERT that is correct, but am unclear on what that ASSERT would look like. (See my comment above!)
Comment 22 youenn fablet 2019-03-01 10:34:22 PST
I filed bug 195216 to follow up on the ASSERT issue.