Bug 8980 – ASSERTION FAILED: !isLoaded (WebKit/WebKit/Plugins/WebBasePluginPackage.m:228 -[WebBasePluginPackage dealloc])

• Component: Plug-ins
• Status: RESOLVED FIXED

[William Coldwell (Cryo)]

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

[Alexey Proskuryakov]

I also saw this once (on the same test), couldn't reproduce. Dual-processor G4.

[Alexey Proskuryakov]

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.

[Dave MacLachlan]

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.

[Darin Adler]

<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.

[Tim Omernick]

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.

[Tim Omernick]

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

[Tim Omernick]

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.

[Tim Omernick]

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.

[Tim Omernick]

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.

[Tim Omernick]

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.

[Darin Adler]

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.

[Tim Omernick]

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.