WWDC session videos don’t play at developer.apple.com
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.
13197014
Created attachment 198825 [details] Patch
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.
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
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
Created attachment 198858 [details] Patch Wrapped the entirety of CookieStorageShim in ENABLE(NETWORK_PROCESS) checks to insulate qt-wk2 from these changes.
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.
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
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.
> >> 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 :)
(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.
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.
(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.
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.
(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 :(
Comment on attachment 198899 [details] Patch Attachment 198899 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/56338
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.
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.
Comment on attachment 198905 [details] Patch Attachment 198905 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/56350
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.
Committed r148782: <http://trac.webkit.org/changeset/148782>
Rolled out in http://trac.webkit.org/changeset/148783. Re-opening.
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?
Created attachment 199114 [details] Patch Fixed Mac build.
The approach is fine with me.
Comment on attachment 199114 [details] Patch Attachment 199114 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/141566
Created attachment 199122 [details] Patch Re-added SecItemShim.dylib to the dependency list for NetworkProcess.
Comment on attachment 199114 [details] Patch Not a formal review, but looks perfectly fine to me.
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.
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.
Committed r149074: <http://trac.webkit.org/changeset/149074>