fast/dom/Window .================= ASSERTION FAILED: !isLoaded (/Volumes/Ernie/WebKit/WebKit/Plugins/WebBasePluginPackage.m:228 -[WebBasePluginPackage dealloc]) ================= Segmentation fault LEAK: 82 Node LEAK: 25 RenderObject LEAK: 1 Frame fast/dom/Window/Plug-ins.html -> failed .Broken pipe
I also saw this once (on the same test), couldn't reproduce. Dual-processor G4.
Saw it again today: 0 com.apple.WebKit 0x0022868c -[WebBasePluginPackage dealloc] + 100 (WebBasePluginPackage.m:230) 1 com.apple.Foundation 0x9290e968 NSPopAutoreleasePool + 536 2 com.apple.Foundation 0x92962b60 -[NSURLConnection(NSURLConnectionInternal) _sendCallbacks] + 724 3 com.apple.Foundation 0x92962810 _sendCallbacks + 156 4 com.apple.CoreFoundation 0x907dc4cc __CFRunLoopDoSources0 + 384 5 com.apple.CoreFoundation 0x907db9fc __CFRunLoopRun + 452 6 com.apple.CoreFoundation 0x907db47c CFRunLoopRunSpecific + 268 7 com.apple.Foundation 0x92941164 -[NSRunLoop runMode:beforeDate:] + 172 8 DumpRenderTree 0x00009a84 dumpRenderTree + 1000 (DumpRenderTree.m:755) Fixing the assignment field.
Ran into this again with Revision 15580. fast/dom/Window .================= ASSERTION FAILED: !isLoaded (/Users/Shared/src/WebKit/WebKit/Plugins/WebBasePluginPackage.m:230 -[WebBasePluginPackage dealloc]) ================= Segmentation fault LEAK: 118 Node LEAK: 25 RenderObject LEAK: 1 Frame fast/dom/Window/Plug-ins.html -> failed .Broken pipe When I reran the tests it was gone. MacBookPro.
<rdar://problem/4526052>
In a (the) failing scenario, -[WebPluginDatabase refresh] is invoked by the test (fast/dom/Window/Plug-ins.html). It proceeds to allocate and initialize a new WebBasePluginPackage for each unique valid plugin in the plugin path. The new WebBasePluginPackages are put in an NSMutableSet called newPlugins. The set of plugins already in the database is then subtracted from newPlugins. For this to be meaningful, equality testing for plugin packages is not based on pointer comparison but rather on name and modification date. Consequently, the newly allocated and initialized plugin packages that correspond to packages that were already in the database are eventually deallocated. It is during the deallocation of such a package that the assertion fails. The reason for that is that during plugin package initialization, the plugin may be loaded. Specifically, it happens under -getPluginInfoFromPLists when the plugin's preferences plist is missing, not localized, or for some reason isn't valid. That's when the plugin is loaded and asked to create the plist (in -pListForPath:createFile:). While it might be interesting to understand why the plist becomes invalid while running the layout tests, it's something that can happen under normal use (such as if the user deletes the plist while the browser is running), so I think this needs to be fixed in WebKit. I think that the best fix is for -pListForPath:createFile: to unload the plugin before returning if it wasn't loaded on entry.
That sounds correct to me. Will fix soon, or review if you want to submit a patch.
Created attachment 9658 [details] Don't leave the package loaded on return from -pListForPath:createFile: if it wasn't loaded on entry I think this patch also makes sense from a performance point of view: you don't want to hold on to for plugins only because you happened to need them to create their plists during database initializtion or refresh.
Comment on attachment 9658 [details] Don't leave the package loaded on return from -pListForPath:createFile: if it wasn't loaded on entry I don't think this is the right fix. If -pListForPath: is the first method to call -load, then the plug-in will be unnecessarily loaded/unloaded once for all legitimate plug-ins. Maybe -[WebPluginDatabase refresh] should explicitly unload the "throw-away" packages
(In reply to comment #8) > (From update of attachment 9658 [details] [edit]) > I don't think this is the right fix. > > If -pListForPath: is the first method to call -load, then the plug-in will be > unnecessarily loaded/unloaded once for all legitimate plug-ins. That's "all legitimate plug-ins that don't have a vaild plist at that time", which is typically a significantly smaller set, I think. But I'll try the other approach!
Created attachment 9660 [details] &Unload those ;throw-away" packages
Comment on attachment 9660 [details] &Unload those ;throw-away" packages Mitz and I talked about this on IRC. This is kind of almost correct, but would do the wrong thing for plugins that are already in the plugin database. In that case you would not want to unload those plugins. I think the best approach would be to ensure that the "throw-away" plugins (ones representing plugins already in the database) are never loaded in the first place. It's a little odd that our plugin packages are "lazily loaded", but they do their lazy loading in -init.
Comment on attachment 9658 [details] Don't leave the package loaded on return from -pListForPath:createFile: if it wasn't loaded on entry I changed my mind about this patch. I think this is actually a reasonable fix. I was initially concerned about the efficiency of loading/unloading the package inside of this method, esp. where the package will eventually be used (and will therefore need to be loaded again). However, looking at the patch again, I see that this is only an issue when the plug-in has an external MIME type registration plist. I am going to test this patch a bit and then land it.
(In reply to comment #12) > (From update of attachment 9658 [details] [edit]) > I changed my mind about this patch. I think this is actually a reasonable fix. > I was initially concerned about the efficiency of loading/unloading the > package inside of this method, esp. where the package will eventually be used > (and will therefore need to be loaded again). > > However, looking at the patch again, I see that this is only an issue when the > plug-in has an external MIME type registration plist. > > I am going to test this patch a bit and then land it. > I thought you'd said that this approach is wrong because the two WebPluginPackage objects reference the same CFBundle, so unloading the bundle from the "temporary" WebPluginPackage would cause it to be unloaded for the "permanent" one, causing a crash on the next call to the plugin.
Comment on attachment 9658 [details] Don't leave the package loaded on return from -pListForPath:createFile: if it wasn't loaded on entry Heh, yeah, you're right. That original patch doesn't guard against closing a plug-in package while an instance is using it.
Created attachment 10047 [details] Huge scary patch Cleanup and rewrite of WebPluginDatabase: - When refreshing the plugin database, we will no longer create "throw-away" plug-in packages for plug-ins that are already in the database. Plug-in packages are now uniqued by path. - Cleaned up the weird load/unload/isLoaded stuff the plug-in package classes. Only Netscape plug-ins can truly be "unloaded". Once a WebKit plug-in bundle has been loaded, there is no way to unload its bundle. - Netscape plug-ins packages can once again be safely removed from the database at runtime. I broke this when I made the change to explicitly unload plug-ins packages when they are removed from the database. A plug-in package is now unloaded after it is removed from the database, AND all its instances have been stopped. - Netscape plug-ins will now properly get an NPP_Shutdown() when the application quits. The browser is required to call this function just before unloading Netscape plug-ins. - document.plugins.refresh() is now 11% faster in the cold case (never scanned for plug-ins), and 22% faster in the warm case.
Comment on attachment 10047 [details] Huge scary patch One thing that must be fixed, that __LP64__ ifndef needs a semicolon outside it. Otherwise, r=me.
Fix landed to TOT WebKit, revision 15896. Repro steps for Safari: 1) Go to Help -> Installed Plug-Ins. 2) Remove ~/Library/Preferences/com.apple.quicktime.plugin.preferences.plist 3) Reload the "Installed Plug-Ins" page.