Bug 220065 – REGRESSION (r261157): Crash in WKSelectPopover when running as iPhone app on iPad

• Component: Forms
• Status: RESOLVED FIXED

[Aditya Keerthi]

...

[Aditya Keerthi]

<rdar://problem/71932792>

[Aditya Keerthi]

Created attachment 416619 [details]
Patch

[Darin Adler]

Comment on attachment 416619 [details]
Patch

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

I’m doing a review+ here but I have some pretty serious concerns so bordering on a review- maybe.

> Source/WebKit/UIProcess/ios/WKContentView.mm:218
> +    if (![UIApplication sharedApplication])
> +        [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(_applicationDidFinishLaunching:) name:UIApplicationDidFinishLaunchingNotification object:nil];

Does not seem right to register for this separately for each WKContentView. WebKit only needs to be notified *once*, and we would then probably want to tell all web *processes* about the observed change, not all web *pages*. Not even sure that initializing WKContentView is the exact right bottleneck for this. We need this registration if we ever stored the result of currentUserInterfaceIdiomIsPadOrMac somewhere while [UIApplication sharedApplication]) returns nil, which may *currently* always be tied to allocating WKContentView objects, but that might be an unnecessarily fragile thing to depend on longer term.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1651
> +    shouldRecommendDesktopClassBrowsing = WTF::nullopt;

This needs to be done once globally, not separately by each web page.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1656
> +    m_preferences->setAllowsInlineMediaPlayback(isPadOrMac);
> +    m_preferences->setInlineMediaPlaybackRequiresPlaysInlineAttribute(!isPadOrMac);
> +    m_preferences->setAllowsInlineMediaPlaybackAfterFullscreen(!isPadOrMac);

Does this do the right thing for textAutosizingUsesIdempotentMode? I think likely not.

Seems risky that this code here is far away from both the WebPreferencesInternal.yaml and -[WKWebViewConfiguration init], since they need to be kept in sync. At least there should be comments pointing from one to the other, since two different bits of code are trying to do the same thing.

But also, the fact that this modifies preferences directly and not WKWebViewConfiguration points to the fact that this is a different semantic from the existing code. The existing code sets a *default* for a newly created WKWebViewConfiguration object. But change will *override* the current setting. So if a program sets the allowsInlineMediaPlayback flag on their WKWebViewConfiguration explicitly, for example, this will ignore that setting and override any change they have made. That’s not consistent with what we do otherwise and not really great; it’s also not great that the configuration will be "wrong" if they read the WKWebViewConfiguration, so that’s another small our solution is imperfect.

Ideally we should think bout the desired behavior when someone customizes the configuration. Or more generally what we’d want here, if we weren’t working around our ability to function properly before UIApplication is initialized.

I think desired behavior would be to have the correct value even if WebKit code is called super-early, rather than being wrong for a while and trying to fix things after the fact, but I suppose that might be difficult. Worth talking through with UIKit team?

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1659
> +    if (hasRunningProcess())
> +        m_process->send(Messages::WebPage::UserInterfaceIdiomDidChange(isPadOrMac), m_webPageID);

Inelegant to do this for every web page: it needs to be done once for every web *process*. Ideally could be a WebProcessProxy function instead of a WebPageProxy function.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1663
> +}
> +
>  
>  } // namespace WebKit

Extra blank line here.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3977
> +    WebKit::setCurrentUserInterfaceIdiomIsPadOrMac(isPadOrMac);

Definitely "once per process" code, not "once per page".

> Tools/TestWebKitAPI/Tests/ios/UserInterfaceIdiomUpdate.mm:51
> +    [webView stringByEvaluatingJavaScript:@"select.focus()"];

This test doesn’t check any of these:

    setAllowsInlineMediaPlayback
    setInlineMediaPlaybackRequiresPlaysInlineAttribute
    setAllowsInlineMediaPlaybackAfterFullscreen

So it would pass even if the code from this patch was wrong. You can delete one or more of those calls and no test would fail. That means we probably need more thorough testing.

[Aditya Keerthi]

(In reply to Darin Adler from comment #3)
> Comment on attachment 416619 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416619&action=review
> 
> I’m doing a review+ here but I have some pretty serious concerns so
> bordering on a review- maybe.
> 
> > Source/WebKit/UIProcess/ios/WKContentView.mm:218
> > +    if (![UIApplication sharedApplication])
> > +        [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(_applicationDidFinishLaunching:) name:UIApplicationDidFinishLaunchingNotification object:nil];
> 
> Does not seem right to register for this separately for each WKContentView.
> WebKit only needs to be notified *once*, and we would then probably want to
> tell all web *processes* about the observed change, not all web *pages*. Not
> even sure that initializing WKContentView is the exact right bottleneck for
> this. We need this registration if we ever stored the result of
> currentUserInterfaceIdiomIsPadOrMac somewhere while [UIApplication
> sharedApplication]) returns nil, which may *currently* always be tied to
> allocating WKContentView objects, but that might be an unnecessarily fragile
> thing to depend on longer term.
> 
> > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1651
> > +    shouldRecommendDesktopClassBrowsing = WTF::nullopt;
> 
> This needs to be done once globally, not separately by each web page.
> 
> > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1656
> > +    m_preferences->setAllowsInlineMediaPlayback(isPadOrMac);
> > +    m_preferences->setInlineMediaPlaybackRequiresPlaysInlineAttribute(!isPadOrMac);
> > +    m_preferences->setAllowsInlineMediaPlaybackAfterFullscreen(!isPadOrMac);
> 
> Does this do the right thing for textAutosizingUsesIdempotentMode? I think
> likely not.
> 
> Seems risky that this code here is far away from both the
> WebPreferencesInternal.yaml and -[WKWebViewConfiguration init], since they
> need to be kept in sync. At least there should be comments pointing from one
> to the other, since two different bits of code are trying to do the same
> thing.
> 
> But also, the fact that this modifies preferences directly and not
> WKWebViewConfiguration points to the fact that this is a different semantic
> from the existing code. The existing code sets a *default* for a newly
> created WKWebViewConfiguration object. But change will *override* the
> current setting. So if a program sets the allowsInlineMediaPlayback flag on
> their WKWebViewConfiguration explicitly, for example, this will ignore that
> setting and override any change they have made. That’s not consistent with
> what we do otherwise and not really great; it’s also not great that the
> configuration will be "wrong" if they read the WKWebViewConfiguration, so
> that’s another small our solution is imperfect.
> 
> Ideally we should think bout the desired behavior when someone customizes
> the configuration. Or more generally what we’d want here, if we weren’t
> working around our ability to function properly before UIApplication is
> initialized.
> 
> I think desired behavior would be to have the correct value even if WebKit
> code is called super-early, rather than being wrong for a while and trying
> to fix things after the fact, but I suppose that might be difficult. Worth
> talking through with UIKit team?

I spoke to the UIKit team, and was told that UIKit provides no guarantee on correctness of any UIKit objects created prior to UIApplicationMain. For the that reason, we cannot guarantee correctness of WKWebViews created prior to UIApplicationMain.

For this reason, I am going to simplify the patch to just fix the crash (which is a regression), and leave the configuration as-is.

I will also make the change per-process rather than per-page.

[Darin Adler]

(In reply to Aditya Keerthi from comment #4)
> I spoke to the UIKit team, and was told that UIKit provides no guarantee on
> correctness of any UIKit objects created prior to UIApplicationMain. For the
> that reason, we cannot guarantee correctness of WKWebViews created prior to
> UIApplicationMain.
> 
> For this reason, I am going to simplify the patch to just fix the crash
> (which is a regression), and leave the configuration as-is.
> 
> I will also make the change per-process rather than per-page.

Seems like we should do some other things:

1) document that you must not create a WKWebViewConfiguration until after UIApplicationMain

2) linked-on-or-after some new iOS, throw an exception if you do that wrong, like we do with main thread violations

But I’m not sure how to do that and still support using WebView in daemons.

[Aditya Keerthi]

Created attachment 417031 [details]
Patch

[Aditya Keerthi]

(In reply to Darin Adler from comment #5)
> (In reply to Aditya Keerthi from comment #4)
> > I spoke to the UIKit team, and was told that UIKit provides no guarantee on
> > correctness of any UIKit objects created prior to UIApplicationMain. For the
> > that reason, we cannot guarantee correctness of WKWebViews created prior to
> > UIApplicationMain.
> > 
> > For this reason, I am going to simplify the patch to just fix the crash
> > (which is a regression), and leave the configuration as-is.
> > 
> > I will also make the change per-process rather than per-page.
> 
> Seems like we should do some other things:
> 
> 1) document that you must not create a WKWebViewConfiguration until after
> UIApplicationMain
> 
> 2) linked-on-or-after some new iOS, throw an exception if you do that wrong,
> like we do with main thread violations
> 
> But I’m not sure how to do that and still support using WebView in daemons.

Something that we can consider – separate from this fix for a regression – throw an exception if a WKWebView/WKWebViewConfiguration is created before UIApplicationMain and UIApplicationMain is then called.

That way, we'd still support using WKWebView in daemons. The downside is that the exception would only be thrown after UIApplicationMain and not when the object is created.

[Darin Adler]

(In reply to Aditya Keerthi from comment #7)
> That way, we'd still support using WKWebView in daemons. The downside is
> that the exception would only be thrown after UIApplicationMain and not when
> the object is created.

Not sure it’s practical, but I do like that idea.

There might be a reason we shouldn’t throw an exception from WebKit code that registers for a notification and expect it to throw through UIApplicationMain back to the app. That is not quite as clean as throwing an exception from a method that was called. But if not, seems like it would accomplish the goal.

Also, just as Address Sanitizer reports where a block was freed, the developer would probably want to see the backtrace of where they did the wrong thing. It would be neat if we could find a way to store that backtrace and include it in the crash report.

[Aditya Keerthi]

I've filed <rdar://problem/72835559> to track the exception work.

[EWS]

Committed r271190: <https://trac.webkit.org/changeset/271190>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417031 [details].