These are the relevant bug details. Note that although a workaround is possible, namely: access NPStream's url field, it is a little kludgy and this in general is a 4xp (4.x compatibility) issue with the Moz' compatible plugin API. Comments from Eric Carlson: The most serious bug for me is a change in the way that the plug-in function NPP_StreamAsFile works. One of the parameters to the function is a C string with the "file://" url the plug-in should load. NS 3.0 sometimes passes a NULL for this param (apparently when it doesn't like the url encoding), but NS 4.x always passes the encoded url. NS 6 seems to always pass a zero length string for the url (0 in the string's first byte), so the QuickTime plug-in fails to load the file. The url _is_ in the NPStream's "url" field, but our plug-in does not look there because all previous versions of NS (and IE for that matter) pass it to the function directly.

peterl and shrir, given the extra engineering help, see if this ought to be a branch check in (if possible). peterl may have bird in hand w.r.t a fix.

This is very easy to fix...a problem in our implmentation. I just need a testcase or steps to reproduce to verify that I have indeed fixed this.

Here's a starter patch: ns4xPluginStreamListener::OnFileAvailable(nsIPluginStreamInfo* pluginInfo, const char* fileName) { if(!mInst || !mInst->IsStarted()) return NS_ERROR_FAILURE; NPP npp; const NPPluginFuncs *callbacks = nsnull; mInst->GetCallbacks(&callbacks); mInst->GetNPP(&npp); if(!callbacks || !npp->pdata) return NS_ERROR_FAILURE; - - pluginInfo->GetURL(&mNPStream.url); + + pluginInfo->GetURL(&mNPStream.url); + + if (mNPStream.url && !fileName) + fileName = PL_strcpy(mNPStream.url); if (callbacks->asfile == NULL) return NS_OK; PRLibrary* lib = nsnull; lib = mInst->fLibrary; NS_TRY_SAFE_CALL_VOID(CallNPP_StreamAsFileProc(callbacks->asfile, npp, &mNPStream, fileName), lib); return NS_OK; }

so there's a tad bit of redundancy, right? we returned the url in NPStream as well as now directly via the function. i guess this is what 4.x did though.

This is an important API compatibility we need to fix for mozilla0.9.2

The following patch will fix the problem. It will also fix loading Acrobat file locally from the hard drive through File | Open: c:\Program Files\Adobe\Acrobat 4.0\Reader\readMe.pdf I wish there was an easier way to do this. Any ideas?

r=av

Waterson, can I get a super review?

General question: how does this fix the bug? Can you walk me through the problem? Nits: - You leak |fileName| when you go through the funky new logic you've added. - Why do you QI() from nsIFile to nsILocalFile, and then back again to nsIFile? I presume you're trying to figure out if |file| is-a nsILocalFile? But if so, why do the extra QI() back? Why do you care if it's an nsILocalFile, anyway? The method you want to call is on the nsIFile interface At a minimum, this needs some serious commenting, because it's pretty zany. - Place your variable declarations as close as possible to where they are being used (e.g., |nsCOMPtr<nsIURI> uri| and |nsresult rv| can be nested inside the first |if| block. - Use NS_CONST_CAST() instead of C-style cast

I'm trying to convert a file://c|/temp string to a c:\temp string. I don't know what I was thinking with all those extra QI's. And I think I plugged the leak. Let me know what you think.

Ok, the new patch looks pretty good. Couple _more_ nits ;-) - You don't need |PRBool createdHere|, because if |fileName && fileName != aFileName|, then you must've allocated it in |GetPath()| - In this block: + nsCOMPtr<nsIFileURL> fileURL = do_QueryInterface(uri, &rv); + nsCOMPtr<nsIFile> file; + if (NS_SUCCEEDED(rv)) + if (NS_SUCCEEDED(fileURL->GetFile(getter_AddRefs(file))) && file) You're a bit inconsistent about handling return codes. I'd personally find this to be sufficient: nsCOMPtr<nsIFileURL> fileURL = do_QueryInterface(uri); if (fileURL) { nsCOMPtr<nsIFile> file; fileURL->GetFile(getter_AddRefs(file)); if (file) { ... } } because nsCOMPtr is known to initialize the pointer to 0. But, if you're feeling ultra-paranoid, then maybe: nsCOMPtr<nsIFileURL> fileURL = do_QueryInterface(uri, &rv); if (NS_SUCCEEDED(rv) && fileURL) { ... } to be consistent with the paranoia below? Heck, get rid of return code paranoia! Just check the pointers to make sure they're non-zero! ;-) (Actually, I don't really care if you make that change or not. It's a stylistic thing, and the couple of extra instructions aren't going to make or break us. It's your code. Go with whatever feels best to you.) So, get rid of |createdHere|, and optionally do the other stuff, and sr=waterson.

This one-line solves the problem one method up the stack.

Boioioioing! Yeah, much nicer! (r|sr)=waterson, whichever helps.

Andrei or Doug, can I get your (r|sr)=?

Cool! r=av

a=dbaron for trunk checkin (on behalf of drivers)

Patch checked in, marking FIXED. /cvsroot/mozilla/modules/plugin/nglsrc/nsPluginHostImpl.cpp new revision: 1.261; previous revision: 1.260

really marking fixed this time.

verif on 0624 windows, local pdf loads now.