Bug 114858 – WWDC session videos don’t play at developer.apple.com

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

[Jer Noble]

WWDC session videos don’t play at developer.apple.com

[Jer Noble]

AVFoundation no longer has access to our session-scoped cookie store when loading is done out of the WebProcess. Until such a time as they can provide API when about to make an HTTP request, interpose at the point where they extract the cookie string from the cookie store, and retrieve the cookies from the network process.

[Jer Noble]

13197014

[Jer Noble]

Created attachment 198825 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/mac/CookieStorageShim.cpp', u'Source/WebKit2/Shared/mac/CookieStorageShim.h', u'Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp', u'Source/WebKit2/Shared/mac/CookieStorageShimLibrary.h', u'Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/EntryPoint/mac/LegacyProcess/WebContentProcessMain.mm', u'Source/WebKit2/WebProcess/EntryPoint/mac/XPCService/WebContentService/Info.plist', u'Source/WebKit2/WebProcess/EntryPoint/mac/XPCService/WebContentServiceEntryPoint.mm', u'Source/WebKit2/WebProcess/WebProcess.cpp']" exit_code: 1
Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp:32:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 11 files


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

[Early Warning System Bot]

Comment on attachment 198825 [details]
Patch

Attachment 198825 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/140353

[Build Bot]

Comment on attachment 198825 [details]
Patch

Attachment 198825 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/33252

[Jer Noble]

Created attachment 198858 [details]
Patch

Wrapped the entirety of CookieStorageShim in ENABLE(NETWORK_PROCESS) checks to insulate qt-wk2 from these changes.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/mac/CookieStorageShim.cpp', u'Source/WebKit2/Shared/mac/CookieStorageShim.h', u'Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp', u'Source/WebKit2/Shared/mac/CookieStorageShimLibrary.h', u'Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/EntryPoint/mac/LegacyProcess/WebContentProcessMain.mm', u'Source/WebKit2/WebProcess/EntryPoint/mac/XPCService/WebContentService/Info.plist', u'Source/WebKit2/WebProcess/EntryPoint/mac/XPCService/WebContentServiceEntryPoint.mm', u'Source/WebKit2/WebProcess/WebProcess.cpp']" exit_code: 1
Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp:34:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 11 files


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

[Eric Carlson]

Comment on attachment 198858 [details]
Patch

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

> Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp:41
> +static CookieStorageShimCallbacks cookieStorageShimCallbacks;

I think we usually wrap shared static variables like this in accessor functions.

> Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp:65
> +        // Protect against accidental recursion:
> +        if (ShimProtector::count() > 1)
> +            break;

If this not expected, it probably be worth adding an ASSERT

[Brady Eidson]

Comment on attachment 198858 [details]
Patch

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

> Source/WebKit2/Shared/mac/CookieStorageShim.cpp:50
> +    KURL firstPartyURL;
> +    if (!WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::CookiesForDOM(false, firstPartyURL, inRequestURL), Messages::NetworkConnectionToWebProcess::CookiesForDOM::Reply(cookies), 0))

Should have a FIXME about actually handling the first party properly if it ever becomes possible.

Also, maybe "firstPartyForCookiesURL" would be better, as "firstPartyForCookies" is the name of the concept used everywhere else.

> Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp:35
> +#define DYLD_INTERPOSE(_replacement, _replacee) \
> +__attribute__((used)) static struct { const void* replacement; const void* replacee; } _interpose_##_replacee \
> +__attribute__ ((section ("__DATA,__interpose"))) = { (const void*)(unsigned long)&_replacement, (const void*)(unsigned long)&_replacee };

Nit - We have the DYLD_INTERPOSE macro in multiple places now.  Maybe a shared header for it would help keep things clean.

>> Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp:41
>> +static CookieStorageShimCallbacks cookieStorageShimCallbacks;
> 
> I think we usually wrap shared static variables like this in accessor functions.

This is just a struct with a single POD member - No runtime cost incurred.

We only wrap shared static variables in an accessor function if they need runtime initializing.

>> Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp:65
>> +            break;
> 
> If this not expected, it probably be worth adding an ASSERT

Yes, please do.

[Brady Eidson]

> >> Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp:41
> >> +static CookieStorageShimCallbacks cookieStorageShimCallbacks;
> > 
> > I think we usually wrap shared static variables like this in accessor functions.
> 
> This is just a struct with a single POD member - No runtime cost incurred.
> 
> We only wrap shared static variables in an accessor function if they need runtime initializing.

Where "needs runtime initializing" either means running constructors, such as with C++ objects, or "the object takes a lot of memory and might not always be needed therefore should only be created on demand"

I think this qualifies as small, always needed, and doesn't have initializers :)

[Eric Carlson]

(In reply to comment #11)
> > >> Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp:41
> > >> +static CookieStorageShimCallbacks cookieStorageShimCallbacks;
> > > 
> > > I think we usually wrap shared static variables like this in accessor functions.
> > 
> > This is just a struct with a single POD member - No runtime cost incurred.
> > 
> > We only wrap shared static variables in an accessor function if they need runtime initializing.
> 
> Where "needs runtime initializing" either means running constructors, such as with C++ objects, or "the object takes a lot of memory and might not always be needed therefore should only be created on demand"
> 
> I think this qualifies as small, always needed, and doesn't have initializers :)

Actually, I think we need to protect access to this with a mutex because we have no guarantee that CoreMedia will alway read cookies from the same thread.

[Jer Noble]

Created attachment 198899 [details]
Patch

Moved the DYLD_INTERPOSE macro into its own header, added an ASSERT in the case of accidental recursion, and renamed a parameter.

[Jer Noble]

(In reply to comment #12)
> (In reply to comment #11)
> > > >> Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp:41
> > > >> +static CookieStorageShimCallbacks cookieStorageShimCallbacks;
> > > > 
> > > > I think we usually wrap shared static variables like this in accessor functions.
> > > 
> > > This is just a struct with a single POD member - No runtime cost incurred.
> > > 
> > > We only wrap shared static variables in an accessor function if they need runtime initializing.
> > 
> > Where "needs runtime initializing" either means running constructors, such as with C++ objects, or "the object takes a lot of memory and might not always be needed therefore should only be created on demand"
> > 
> > I think this qualifies as small, always needed, and doesn't have initializers :)
> 
> Actually, I think we need to protect access to this with a mutex because we have no guarantee that CoreMedia will alway read cookies from the same thread.

Ooh, good point.  I'll add a dispatch_once here.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm', u'Source/WebKit2/Shared/mac/CookieStorageShim.cpp', u'Source/WebKit2/Shared/mac/CookieStorageShim.h', u'Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp', u'Source/WebKit2/Shared/mac/CookieStorageShimLibrary.h', u'Source/WebKit2/Shared/mac/DYLDInterpose.h', u'Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/EntryPoint/mac/LegacyProcess/WebContentProcessMain.mm', u'Source/WebKit2/WebProcess/EntryPoint/mac/XPCService/WebContentService/Info.plist', u'Source/WebKit2/WebProcess/EntryPoint/mac/XPCService/WebContentServiceEntryPoint.mm', u'Source/WebKit2/WebProcess/WebProcess.cpp', u'Source/WebKit2/WebProcess/mac/SecItemShimLibrary.mm']" exit_code: 1
Source/WebKit2/Shared/mac/DYLDInterpose.h:34:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 14 files


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

[Brady Eidson]

(In reply to comment #12)
> (In reply to comment #11)
> > > >> Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp:41
> > > >> +static CookieStorageShimCallbacks cookieStorageShimCallbacks;
> > > > 
> > > > I think we usually wrap shared static variables like this in accessor functions.
> > > 
> > > This is just a struct with a single POD member - No runtime cost incurred.
> > > 
> > > We only wrap shared static variables in an accessor function if they need runtime initializing.
> > 
> > Where "needs runtime initializing" either means running constructors, such as with C++ objects, or "the object takes a lot of memory and might not always be needed therefore should only be created on demand"
> > 
> > I think this qualifies as small, always needed, and doesn't have initializers :)
> 
> Actually, I think we need to protect access to this with a mutex because we have no guarantee that CoreMedia will alway read cookies from the same thread.

Oh man that sucks  :(

[Build Bot]

Comment on attachment 198899 [details]
Patch

Attachment 198899 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/56338

[Jer Noble]

Created attachment 198905 [details]
Patch

Made the m_count static of ShimProtector thread-local, so it will protect against re-entrancy within a thread, but not across threads. Also made the initializer function assert if you attempt to initialize it twice.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm', u'Source/WebKit2/Shared/mac/CookieStorageShim.cpp', u'Source/WebKit2/Shared/mac/CookieStorageShim.h', u'Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp', u'Source/WebKit2/Shared/mac/CookieStorageShimLibrary.h', u'Source/WebKit2/Shared/mac/DYLDInterpose.h', u'Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/EntryPoint/mac/LegacyProcess/WebContentProcessMain.mm', u'Source/WebKit2/WebProcess/EntryPoint/mac/XPCService/WebContentService/Info.plist', u'Source/WebKit2/WebProcess/EntryPoint/mac/XPCService/WebContentServiceEntryPoint.mm', u'Source/WebKit2/WebProcess/WebProcess.cpp', u'Source/WebKit2/WebProcess/mac/SecItemShimLibrary.mm']" exit_code: 1
Source/WebKit2/Shared/mac/DYLDInterpose.h:34:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 14 files


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

[Build Bot]

Comment on attachment 198905 [details]
Patch

Attachment 198905 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/56350

[Eric Carlson]

Comment on attachment 198905 [details]
Patch

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

> Source/WebKit2/Shared/mac/CookieStorageShimLibrary.cpp:33
> +extern "C" CFDictionaryRef _CFHTTPCookieStorageCopyRequestHeaderFieldsForURL(CFAllocatorRef inAllocator, CFHTTPCookieStorageRef inCookieStorage, CFURLRef inRequestURL);

Nit: the parameter names shouldn't be necessary.

[Jer Noble]

Committed r148782: <http://trac.webkit.org/changeset/148782>

[Jer Noble]

Rolled out in http://trac.webkit.org/changeset/148783.  Re-opening.

[Mark Rowe (bdash)]

Comment on attachment 198905 [details]
Patch

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

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:29
> +#import "DYLDInterpose.h"

I'm really not a fan of having "DYLD" as uppercase like that. It's not an acronym or initialism. Maybe rename the header to something like SymbolInterposing / LoadTimeInterposing / Interposing?

[Jer Noble]

Created attachment 199114 [details]
Patch

Fixed Mac build.

[Brady Eidson]

The approach is fine with me.

[Build Bot]

Comment on attachment 199114 [details]
Patch

Attachment 199114 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/141566

[Jer Noble]

Created attachment 199122 [details]
Patch

Re-added SecItemShim.dylib to the dependency list for NetworkProcess.

[Brady Eidson]

Comment on attachment 199114 [details]
Patch

Not a formal review, but looks perfectly fine to me.

[Eric Carlson]

Comment on attachment 199114 [details]
Patch

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

> Source/WebKit2/Shared/mac/CookieStorageShimLibrary.h:36
> +    CFDictionaryRef (*cookieStorageCopyRequestHeaderFieldsForURL)(CFHTTPCookieStorageRef inCookieStorage, CFURLRef inRequestURL);

Nit: the parameter names shouldn't be necessary.

> Source/WebKit2/Shared/mac/CookieStorageShimLibrary.h:39
> +typedef void (*CookieStorageShimInitializeFunc)(const CookieStorageShimCallbacks& callbacks);

Ditto.

[Eric Carlson]

Comment on attachment 199122 [details]
Patch

I am not a WK2 owner, but marking r+ based on Brady's comments. Please see the nits in the previous version.

[Jer Noble]

Committed r149074: <http://trac.webkit.org/changeset/149074>