Bug 137893 – [Mac][WK2] Fix applicationIsSafari() detection

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

I noticed when profiling Safari on Mac that we were exercising a code patch we shouldn't because it is meant for other applications that Safari.

The detection relies on the applicationIsSafari() function in RuntimeApplicationChecks.cpp. This was in some cases returning false on my machine even though I was running Safari so I investigated a bit and noticed that the check relies on the main bundle identifier string and was doing:
static bool isSafari = mainBundleIsEqualTo("com.apple.Safari") || mainBundleIsEqualTo("com.apple.WebProcess");

The WebProcess detection seems wrong because:
- I believe the WebProcess bundle is called "com.apple.WebKit.WebContent" nowadays, not "com.apple.WebProcess".
- On my machine, since I am using a development build, the name is actually "com.apple.WebKit.WebContent.Development" so we should probably match this too.

Here is a backtrace that lead to isSafari being false:
0               WebCore::ResourceRequest::useQuickLookResourceCachingQuirks()
1   0x10e76c2aa WebCore::FrameLoader::subresourceCachePolicy() const
2   0x10e47c6f4 WebCore::CachedResourceLoader::determineRevalidationPolicy(WebCore::CachedResource::Type, WebCore::ResourceRequest&, bool, WebCore::CachedResource*, WebCore::CachedResourceRequest::DeferOption) const
3   0x10e47b885 WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type, WebCore::CachedResourceRequest&)
4   0x10e47bb95 WebCore::CachedResourceLoader::requestCSSStyleSheet(WebCore::CachedResourceRequest&)
5   0x10e81761c WebCore::HTMLLinkElement::process()
6   0x10e817abb WebCore::HTMLLinkElement::insertedInto(WebCore::ContainerNode&)
7   0x10e4c5de8 WebCore::ChildNodeInsertionNotifier::notify(WebCore::Node&)
8   0x10e4c1c52 WebCore::ContainerNode::parserAppendChild(WTF::PassRefPtr<WebCore::Node>)
9   0x10e7eafb9 WebCore::insert(WebCore::HTMLConstructionSiteTask&)
10  0x10e7e7266 WebCore::HTMLConstructionSite::executeQueuedTasks()
11  0x10e7ef1e6 WebCore::HTMLDocumentParser::constructTreeFromHTMLToken(WebCore::HTMLToken&)
12  0x10e7eed2f WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)
13  0x10e7ef6ad WebCore::HTMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>)
14  0x10e5b4245 WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter&, char const*, unsigned long)
15  0x10e61bcb8 WebCore::DocumentLoader::commitData(char const*, unsigned long)
16  0x10d0e73b7 WebKit::WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int)
17  0x10e61d82f WebCore::DocumentLoader::commitLoad(char const*, int)
18  0x10e61dcad WebCore::DocumentLoader::dataReceived(WebCore::CachedResource*, char const*, int)
19  0x10e474723 WebCore::CachedRawResource::didAddClient(WebCore::CachedResourceClient*)
20  0x10f1bdf6d WebCore::ThreadTimers::sharedTimerFiredInternal()
21  0x10f04b6c4 WebCore::timerFired(__CFRunLoopTimer*, void*)

[Chris Dumez]

On iOS, we seem to use the following check:
static bool isWebProcess = [[[NSBundle mainBundle] bundleIdentifier] isEqualToString:@"com.apple.WebKit.WebContent.Development"] || [[[NSBundle mainBundle] bundleIdentifier] isEqualToString:@"com.apple.WebKit.WebContent"] || [[[NSBundle mainBundle] bundleIdentifier] isEqualToString:@"com.apple.WebProcess"];

Could someone let me know if I need to keep matching "com.apple.WebProcess" as well (in addition to "com.apple.WebKit.WebContent" / "com.apple.WebKit.WebContent.Development"), like iOS does?

[Chris Dumez]

Note that this means we would spend ~1% of the WebProcess cpu time (when loading apple.com) trying to detect if we should useQuickLookResourceCachingQuirks(), even though that check is supposed to be disabled for Safari.

[Chris Dumez]

Created attachment 240145 [details]
Patch

[Darin Adler]

Comment on attachment 240145 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        patch we shouldn't because it is meant for other applications that

Typo: patch -> path

> Source/WebCore/platform/RuntimeApplicationChecks.cpp:63
>  bool applicationIsSafari()
>  {
> -    // FIXME: For the WebProcess case, ensure that this is Safari's WebProcess.
> -    static bool isSafari = mainBundleIsEqualTo("com.apple.Safari") || mainBundleIsEqualTo("com.apple.WebProcess");
> +    // FIXME: For the WebContent case, ensure that this is Safari's WebContent process.
> +    static bool isSafari = mainBundleIsEqualTo("com.apple.Safari") || mainBundleIsEqualTo("com.apple.WebKit.WebContent") || mainBundleIsEqualTo("com.apple.WebKit.WebContent.Development") || mainBundleIsEqualTo("com.apple.WebProcess");
>      return isSafari;
>  }

That FIXME is a really bad problem. This function has lots of false positives. It’s going to return true for tons of uses that are not Safari. Why is this OK?

[Chris Dumez]

+aestes@ who I think added this code.

[Chris Dumez]

Comment on attachment 240145 [details]
Patch

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

>> Source/WebCore/platform/RuntimeApplicationChecks.cpp:63
>>  }
> 
> That FIXME is a really bad problem. This function has lots of false positives. It’s going to return true for tons of uses that are not Safari. Why is this OK?

I don't know :/ Does anyone have an idea how we could differentiate Safari's WebProcesses from other applications' WebProcesses?

[Chris Dumez]

Created attachment 240148 [details]
Patch

[Alexey Proskuryakov]

> Does anyone have an idea how we could differentiate Safari's WebProcesses from other applications' WebProcesses?

We certainly do, in WebKit2.

Having logic based on "is Safari" as low down the stack as ResourceRequest is just crazy. I'd love to find another way to do it. FrameLoader certainly has clients and strategies to call out to WebKit2.

> Could someone let me know if I need to keep matching "com.apple.WebProcess"

com.apple.WebProcess is still supported in WebKit trunk. Whether it's needed for this particular case, I'm not sure.

[Chris Dumez]

Comment on attachment 240148 [details]
Patch

Clearing cq flag for now as Alexey is proposing I refactor the code instead to make sure applicationIsSafari() is called from WebKit instead of WebCore.

[Chris Dumez]

Created attachment 240223 [details]
Alternative WIP patch

Alexey, I tried to refactor the code based on our discussion. Could you please take a look at the WIP patch and let me know if I understood your proposal correctly?

[Chris Dumez]

Created attachment 240228 [details]
Alternative WIP patch

New iteration removing the code out of ResourceRequest and into FrameLoaderClient, as discussed with Alexey on IRC.
I did not know where to put the determineShouldUseQuickLookResourceCachingQuirks() code since Alexey didn't like ResourceRequest and since I'd like to share the code between WK1 and WK2. So I moved it to a new QuickLookMac class under platform/.

[WebKit Commit Bot]

Attachment 240228 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mac/QuickLookMac.mm:51:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 240228 [details]
Alternative WIP patch

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

r=me

> Source/WebCore/loader/FrameLoaderClient.h:289
> +        virtual bool shouldUseQuickLookResourceCachingQuirks() const = 0;

I think that this needsQuickLookResourceCachingQuirks would be a better name for this function.

> Source/WebCore/platform/mac/QuickLookMac.mm:33
> +bool QuickLookMac::determineShouldUseQuickLookResourceCachingQuirks()

Perhaps simply "shouldUseQuickLookResourceCachingQuirks"? Any function that returns a bool determines something, and "determine should" is not correct grammar anyway.

> Source/WebCore/platform/mac/QuickLookMac.mm:38
> +    NSArray* frameworks = [NSBundle allFrameworks];

Misplaced star: should be "NSArray *" (I know that it's moved code).

> Source/WebCore/platform/mac/QuickLookMac.mm:43
> +    for (NSBundle* bundle in frameworks) {

Misplaced star: should be "NSBundle *".

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:194
> +#if !PLATFORM(IOS)

I'm not sure if I always understand why this patch uses !PLATFORM(IOS) in some places, and PLATFORM(MAC) in others. This may be worth another look.

[Chris Dumez]

Comment on attachment 240228 [details]
Alternative WIP patch

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

>> Source/WebCore/platform/mac/QuickLookMac.mm:33
>> +bool QuickLookMac::determineShouldUseQuickLookResourceCachingQuirks()
> 
> Perhaps simply "shouldUseQuickLookResourceCachingQuirks"? Any function that returns a bool determines something, and "determine should" is not correct grammar anyway.

I wanted to hint to the caller that this method is expensive. shouldUseQuickLookResourceCachingQuirks() might should like a simple getter. Maybe computeShouldUseQuickLookResourceCachingQuirks() like in my earlier revision?

[Chris Dumez]

Created attachment 240233 [details]
Patch

[Chris Dumez]

Comment on attachment 240233 [details]
Patch

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

> Source/WebCore/platform/mac/QuickLookMac.h:33
> +    WEBCORE_EXPORT static bool computeNeedsQuickLookResourceCachingQuirks();

I addressed all the comments. However, I named this function computeNeedsQuickLookResourceCachingQuirks() to make it clearer to the caller that this method is potentially expensive and that the result needs to be cached.
Please let me know if you are OK with this before I land.

[Alexey Proskuryakov]

> computeNeedsQuickLookResourceCachingQuirks

OK.

One thing that I now noticed this patch doesn't do is remove the horrendous hack from applicationIsSafari(). Cannot we do that now?

[Chris Dumez]

(In reply to comment #17)
> > computeNeedsQuickLookResourceCachingQuirks
> 
> OK.
> 
> One thing that I now noticed this patch doesn't do is remove the horrendous
> hack from applicationIsSafari(). Cannot we do that now?

Do you mean by checking [[NSBundle mainBundle] bundleIdentifier] at the call sites and only calling computeNeedsQuickLookResourceCachingQuirks() if the bundle name is *not* "com.apple.Safari"?

[Chris Dumez]

(In reply to comment #18)
> (In reply to comment #17)
> > > computeNeedsQuickLookResourceCachingQuirks
> > 
> > OK.
> > 
> > One thing that I now noticed this patch doesn't do is remove the horrendous
> > hack from applicationIsSafari(). Cannot we do that now?
> 
> Do you mean by checking [[NSBundle mainBundle] bundleIdentifier] at the call
> sites and only calling computeNeedsQuickLookResourceCachingQuirks() if the
> bundle name is *not* "com.apple.Safari"?

Oh, you mean removing || mainBundleIsEqualTo("com.apple.WebProcess") from applicationIsSafari. Yes, we can do this now as applicationIsSafari is always called from the UIProcess now.

[Alexey Proskuryakov]

> Oh, you mean removing || mainBundleIsEqualTo("com.apple.WebProcess") from applicationIsSafari

Yes, rs=me to do this as part of this patch.

[Chris Dumez]

Created attachment 240276 [details]
Patch

[Chris Dumez]

(In reply to comment #20)
> > Oh, you mean removing || mainBundleIsEqualTo("com.apple.WebProcess") from applicationIsSafari
> 
> Yes, rs=me to do this as part of this patch.

Done.

[WebKit Commit Bot]

Comment on attachment 240276 [details]
Patch

Clearing flags on attachment: 240276

Committed r175055: <http://trac.webkit.org/changeset/175055>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 240276 [details]
Patch

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

> Source/WebCore/platform/mac/QuickLookMac.mm:46
> +        const char* bundleID = [[bundle bundleIdentifier] UTF8String];
> +        if (bundleID && !strcasecmp(bundleID, "com.apple.QuickLookUIFramework"))
> +            return true;

It’s strange that code was converting an identifier to a UTF-8 string just to compare it with a constant. I would write something using NSString directly and using

    @"com.apple.QuickLookUIFramework"

These flags are handy:

    NSCaseInsensitiveSearch | NSLiteralSearch | NSAnchoredSearch