496344 - Investigate why we keep reopening the Flash resource map

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 397053 comment 30 and bug 397053 comment 33.  We should stop doing that, if possible.

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK, so I do see us opening the .rsrc file the first time I open a page with Flash, but only that once.  That comes when setting up the nsIPlugin the first time:

libgkplugin.dylib`OpenPluginResourceFork(nsIFile*)+0x10c
libgkplugin.dylib`nsPluginFile::OpenPluginResource()+0x19
libgkplugin.dylib`nsNPAPIPlugin::CreatePlugin(char const*, char const*, PRLibrary*, nsIPlugin**)+0xa6

Then again, I'm using Flash 10.... Michelle, any idea where I should be looking for the calls to open the resource fork that you mentioned in bug 397053?  Are they done by the plug-in itself in response to something we do, or by Gecko code?

Comment 2

[Michelle Sintov]

Prior to our Flash Player fix, our investigation showed that the Flash Player was leaking our resources every time a page with Flash content was reloaded. This could be easily repro'ed by reloading a Flash page many times. The opened resource files would keep growing without closing. The problem for us was that when NP_Shutdown was called, we wouldn't close the resource map if it was opened by the browser.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  It looks like we open the resource fork in nsNPAPIPlugin::CreatePlugin and stash the resulting integer identifier it in the nsNSPAPIPlugin object... and the only thing we seem to use it is to CloseResFile() in nsNPAPIPlugin::Shutdown (after making the NP_Shutdown call on the plug-in, which is what bit Flash).

We do call UseResFile() right after opening, but seem to do nothing else interesting with this identifier...

Josh, is there any reason we couldn't just close it after calling UseResFile or something?

Comment 4

[Josh Aas]

Bug 500925 fixes the problem of reloading the Flash resource map frequently, including the case where you have a single page with a Flash instance open and you reload it. Between that and the fix on the Flash side of things I think we're all good here now.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Uh... Except we still open the resource map and don't close it.  We just do it more rarely....

If we don't want to fix that, that's fine.  But then we should be clear about that (and wontfix this bug).

Comment 6

[Josh Aas]

I see us opening the resource map when we load the plugin and closing if when we unload the plugin. I did a bunch of testing before closing this bug and I never saw a mismatch between loads and unloads. Do you have a test case showing a mismatch? You want to look at matching calls for "::CFBundleOpenBundleResourceMap" (used once in the source code) and "::CloseResFile" (used twice).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  I guess what I was really trying to say is that we should try to close the file before NP_Shutdown on the plug-in.  Again, I'm fine if we want to wontfix that.

Comment 8

[Josh Aas]

I don't really want to fix that if it is even possible. Given that this bug's title makes it explicitly about "reopening" the resource map I'm going to close it FIXED. If you want to change the bug to be about the length of time the resource map is open for then we can close this as WONTFIX.