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.
<rdar://problem/46887179>
Created attachment 363150 [details] Patch
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 on attachment 363150 [details] Patch r=me
Comment on attachment 363150 [details] Patch Clearing flags on attachment: 363150 Committed r242181: <https://trac.webkit.org/changeset/242181>
All reviewed patches have been landed. Closing bug.
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
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()
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.
I'm sure this'll be easy to resolve but I will not be able to do so today.
Actually let me see if I can in the next 10 minutes. Hang on.
Have to build for desktop from scratch. Will check on it when I get home and try to resolve this tonight.
(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. =/
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)
Reopening to attach new patch.
Created attachment 363281 [details] Patch
Comment on attachment 363281 [details] Patch Clearing flags on attachment: 363281 Committed r242249: <https://trac.webkit.org/changeset/242249>
> 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.
> 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?
(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!)
I filed bug 195216 to follow up on the ASSERT issue.