Bug 195126 – Universal links from Google search results pages don't open the app

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

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.

[Brady Eidson]

<rdar://problem/46887179>

[Brady Eidson]

Created attachment 363150 [details]
Patch

[EWS Watchlist]

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.

[Geoffrey Garen]

Comment on attachment 363150 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 363150 [details]
Patch

Clearing flags on attachment: 363150

Committed r242181: <https://trac.webkit.org/changeset/242181>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/242181/webkit

caused 3 test crashes on Mac Debug:
fast/xmlhttprequest/xmlhttprequest-no-file-access.html
fast/files/file-reader-file-url.html
security/cannot-read-self-from-file.html

Crash:
https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r242199%20(1759)/fast/files/file-reader-immediate-abort-stderr.txt

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=security%2Fcannot-read-self-from-file.html%20fast%2Fxmlhttprequest%2Fxmlhttprequest-no-file-access.html%20fast%2Ffiles%2Ffile-reader-file-url.html

[Geoffrey Garen]

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

[Ryan Haddad]

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.

[Brady Eidson]

I'm sure this'll be easy to resolve but I will not be able to do so today.

[Brady Eidson]

Actually let me see if I can in the next 10 minutes. Hang on.

[Brady Eidson]

Have to build for desktop from scratch. Will check on it when I get home and try to resolve this tonight.

[Brady Eidson]

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

[Brady Eidson]

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)

[Brady Eidson]

Reopening to attach new patch.

[Brady Eidson]

Created attachment 363281 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 363281 [details]
Patch

Clearing flags on attachment: 363281

Committed r242249: <https://trac.webkit.org/changeset/242249>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

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

[Geoffrey Garen]

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

[Brady Eidson]

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

[youenn fablet]

I filed bug 195216 to follow up on the ASSERT issue.