Bug 137893 - [Mac][WK2] Fix applicationIsSafari() detection
Summary: [Mac][WK2] Fix applicationIsSafari() detection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-20 13:50 PDT by Chris Dumez
Modified: 2014-10-22 18:50 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.69 KB, patch)
2014-10-20 14:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (2.90 KB, patch)
2014-10-20 15:05 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Alternative WIP patch (9.06 KB, patch)
2014-10-21 14:40 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Alternative WIP patch (21.61 KB, patch)
2014-10-21 16:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (28.97 KB, patch)
2014-10-21 17:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (30.08 KB, patch)
2014-10-22 09:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-10-20 13:50:34 PDT
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*)
Comment 1 Chris Dumez 2014-10-20 14:00:02 PDT
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?
Comment 2 Chris Dumez 2014-10-20 14:12:37 PDT
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.
Comment 3 Chris Dumez 2014-10-20 14:16:09 PDT
Created attachment 240145 [details]
Patch
Comment 4 Darin Adler 2014-10-20 14:44:52 PDT
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?
Comment 5 Chris Dumez 2014-10-20 15:01:03 PDT
+aestes@ who I think added this code.
Comment 6 Chris Dumez 2014-10-20 15:04:08 PDT
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?
Comment 7 Chris Dumez 2014-10-20 15:05:53 PDT
Created attachment 240148 [details]
Patch
Comment 8 Alexey Proskuryakov 2014-10-20 16:20:35 PDT
> 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.
Comment 9 Chris Dumez 2014-10-21 13:47:18 PDT
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.
Comment 10 Chris Dumez 2014-10-21 14:40:44 PDT
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?
Comment 11 Chris Dumez 2014-10-21 16:45:33 PDT
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/.
Comment 12 WebKit Commit Bot 2014-10-21 16:48:20 PDT
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.
Comment 13 Alexey Proskuryakov 2014-10-21 17:12:04 PDT
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.
Comment 14 Chris Dumez 2014-10-21 17:35:48 PDT
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?
Comment 15 Chris Dumez 2014-10-21 17:57:33 PDT
Created attachment 240233 [details]
Patch
Comment 16 Chris Dumez 2014-10-21 17:59:38 PDT
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.
Comment 17 Alexey Proskuryakov 2014-10-22 09:38:47 PDT
> 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?
Comment 18 Chris Dumez 2014-10-22 09:49:49 PDT
(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"?
Comment 19 Chris Dumez 2014-10-22 09:52:42 PDT
(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.
Comment 20 Alexey Proskuryakov 2014-10-22 09:56:46 PDT
> Oh, you mean removing || mainBundleIsEqualTo("com.apple.WebProcess") from applicationIsSafari

Yes, rs=me to do this as part of this patch.
Comment 21 Chris Dumez 2014-10-22 09:57:17 PDT
Created attachment 240276 [details]
Patch
Comment 22 Chris Dumez 2014-10-22 09:57:42 PDT
(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.
Comment 23 WebKit Commit Bot 2014-10-22 10:43:06 PDT
Comment on attachment 240276 [details]
Patch

Clearing flags on attachment: 240276

Committed r175055: <http://trac.webkit.org/changeset/175055>
Comment 24 WebKit Commit Bot 2014-10-22 10:43:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Darin Adler 2014-10-22 18:50:02 PDT
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