Bug 173386 - [iOS] Generate a simulated crash when the WebThread starts in MobileSafari
Summary: [iOS] Generate a simulated crash when the WebThread starts in MobileSafari
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-14 15:02 PDT by David Kilzer (:ddkilzer)
Modified: 2017-06-15 17:46 PDT (History)
9 users (show)

See Also:


Attachments
Patch (1.64 KB, patch)
2017-06-14 15:02 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (13.46 KB, patch)
2017-06-14 20:54 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v3 (13.47 KB, patch)
2017-06-15 02:41 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v4 (14.68 KB, patch)
2017-06-15 08:11 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v5 (14.68 KB, patch)
2017-06-15 08:16 PDT, David Kilzer (:ddkilzer)
aestes: review+
Details | Formatted Diff | Diff
Build fix v1 (4.52 KB, patch)
2017-06-15 11:57 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Build fix v2 (4.42 KB, patch)
2017-06-15 12:27 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2017-06-14 15:02:09 PDT
Need the bug URL (OOPS!).
<rdar://problem/32776426>

Reviewed by NOBODY (OOPS!).

* platform/ios/wak/WebCoreThread.mm:
(WebThreadEnable): Add release assert for MobileSafari.
---
 2 files changed, 13 insertions(+), 1 deletion(-)
Comment 1 David Kilzer (:ddkilzer) 2017-06-14 15:02:10 PDT
Created attachment 312923 [details]
Patch
Comment 2 David Kilzer (:ddkilzer) 2017-06-14 15:06:53 PDT
<rdar://problem/32776426>
Comment 3 Andy Estes 2017-06-14 16:22:41 PDT
Actually I’m not sure about this. In general, we intentionally support WKWebView and legacy WebKit in the same app. I’m not sure why MobileSafari should be an exception to this rule.
Comment 4 David Kilzer (:ddkilzer) 2017-06-14 19:46:16 PDT
Comment on attachment 312923 [details]
Patch

After talking to Andy, we're going to switch to use a simulated crash since there might be a mix of expected and unexpected use cases, and we don't want to make MobileSafari unusable until we understand (all) the scenarios where the WebThread becomes active.
Comment 5 David Kilzer (:ddkilzer) 2017-06-14 20:54:50 PDT
Created attachment 312950 [details]
Patch v2
Comment 6 Build Bot 2017-06-14 20:56:05 PDT
Attachment 312950 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/ios/CrashReporterSupportSPI.h:36:  The parameter name "pid" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 David Kilzer (:ddkilzer) 2017-06-15 02:41:10 PDT
Created attachment 312966 [details]
Patch v3
Comment 8 Build Bot 2017-06-15 02:42:56 PDT
Attachment 312966 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/ios/CrashReporterSupportSPI.h:36:  The parameter name "pid" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 David Kilzer (:ddkilzer) 2017-06-15 08:11:46 PDT
Created attachment 312981 [details]
Patch v4
Comment 10 Build Bot 2017-06-15 08:14:20 PDT
Attachment 312981 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/ios/CrashReporterSupportSPI.h:37:  The parameter name "pid" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 David Kilzer (:ddkilzer) 2017-06-15 08:16:34 PDT
Created attachment 312983 [details]
Patch v5
Comment 12 David Kilzer (:ddkilzer) 2017-06-15 08:17:36 PDT
(In reply to David Kilzer (:ddkilzer) from comment #11)
> Created attachment 312983 [details]
> Patch v5

Last one!  Minor header adjustment after fixing build issues in v4.
Comment 13 Build Bot 2017-06-15 08:19:07 PDT
Attachment 312983 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/ios/CrashReporterSupportSPI.h:38:  The parameter name "pid" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Andy Estes 2017-06-15 09:58:35 PDT
Comment on attachment 312983 [details]
Patch v5

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:21096
> +				44CA12971EF23A6500E4B3AD /* CrashReporterSupportSPI.h */,
>  				CE1252421A16C01A00864480 /* CoreUISPI.h */,

Sorting problem.
Comment 15 David Kilzer (:ddkilzer) 2017-06-15 10:05:21 PDT
Committed r218339: <http://trac.webkit.org/changeset/218339>
Comment 16 David Kilzer (:ddkilzer) 2017-06-15 11:22:57 PDT
(In reply to David Kilzer (:ddkilzer) from comment #15)
> Committed r218339: <http://trac.webkit.org/changeset/218339>

This broke internal iOS Simulator builds.  Working on a fix now.
Comment 17 David Kilzer (:ddkilzer) 2017-06-15 11:57:28 PDT
Reopening to attach new patch.
Comment 18 David Kilzer (:ddkilzer) 2017-06-15 11:57:28 PDT
Created attachment 312994 [details]
Build fix v1
Comment 19 David Kilzer (:ddkilzer) 2017-06-15 12:27:13 PDT
Created attachment 312997 [details]
Build fix v2
Comment 20 David Kilzer (:ddkilzer) 2017-06-15 12:30:18 PDT
Committed r218347: <http://trac.webkit.org/changeset/218347>
Comment 21 mitz 2017-06-15 13:16:32 PDT
Comment on attachment 312983 [details]
Patch v5

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

> Source/WebCore/platform/ios/wak/WebCoreThread.mm:1007
> +        SimulateCrash(getpid(), kExceptionCode, @"MobileSafari should never run a WebThread");

This is a strange assertion for a framework to make about one of its clients, and it would have made more sense (and less of a layering violation) to let clients register for a callback when the Web thread is created so that they can enforce whatever policy they might have.

It is also strange for WebKit to assert this about MobileSafari, which is a rather complex app that uses many frameworks in the system which it might not have direct control over, but this is not a good place to get into such details of how Apple develops Safari and iOS.
Comment 22 David Kilzer (:ddkilzer) 2017-06-15 17:46:07 PDT
Okay, reverted this change in:

Committed r218370: <http://trac.webkit.org/changeset/218370>

Moving to RESOLVED/WONTFIX.