Opening from the Download Manager (DM): 1. use today's nightly build 2. Open the DM from Ctrl+j or via Menu 3. right click any file and choose: Open File Location 4. Browser crashes with the stack: https://crash-stats.mozilla.com/report/index/bp-54b73685-d4a7-49b4-8caf-e3d362110909 Win7 x64, m-c nightly from 9/9/11 (32 bit) Blocks bug 670068
Strangely enough this crash does not happen on my compiled build, nor on the Nightly Debug build, but does on the Nightly release build.
Crashes with a new Profile as well, with no addons or history of any sort.
FWIW, the 64-bit build locked up as reported by the Win7 program has stopped running dialog, but no crash.
I sometimes get a hang too when opening the download manager but it's not related to this bug because the code here only executes when you reveal location. Should be posted separately.
Actually there is GlobalInit() which is called on startup which loads the reveal functions dynamically. Let me investigate that first just in case for the hang.
I don't see a problem, but I think the fix is to use CoTaskMemFree instead of ILFree. ILCreateFroMPath explicitly says to use ILFree ( http://msdn.microsoft.com/en-us/library/dd378420(v=vs.85).aspx ) but if you read the remarks of ILFree it says you should use CoTaskMemFree. I think the ILFree is causing some kind of corruption on the heap that causes the later problem with the finalize string call. I'm making a release build now to try to reproduce the problem so I can verify if this is the fix.
Created attachment 559527 [details] [diff] [review] Fixed crash by specifying WINAPI on typedef So the problem ended up being simply that WINAPI was not specified on the function pointer. So the calling convention was different than __stdcall in Release builds. __stdcall is the default calling convention but with optimizations on I think we probably use __fastcall or something similar. http://msdn.microsoft.com/en-us/library/6xa169sk(v=vs.80).aspx I don't think there's any point in running this through try, I'll push directly to mozilla-inbound was I get a review.
Any time I load functions dynamically I'll be trying on release builds from now on :)
> was I get a review. *once I get a review
Comment on attachment 559527 [details] [diff] [review] Fixed crash by specifying WINAPI on typedef > I don't think there's any point in running this through try, I'll push directly > to mozilla-inbound was I get a review. I'd go directly to mozilla-central so it's sure to get into tonight's nightly.
k will do, thanks for the quick review.
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/d078623f7875
Verified fixed using latest hourly based on cset: http://hg.mozilla.org/mozilla-central/rev/d078623f7875
Adding [@ ILFindLastID ] since it appears to be related to this bug and is showing up in crash stats as a new crash.
(In reply to Marcia Knous [:marcia] from comment #15) > Adding [@ ILFindLastID ] since it appears to be related to this bug and is > showing up in crash stats as a new crash. The ILFindLastID crashes are not fixed in today's nightly, whereas the nsAString_internal::Finalize() crashes are.
I notice all of the CPU fields are amd64 does that mean they are using the 64-bit version of the software or that they simply have an amd64 processor and are running the x86 version of Firefox?
Mine is a Win7 64-bit OS running on an Intel Core i3 processor. I had my crashes on the 32-bit version of Nightly. Hope this helps you out.
Re-opening for the second crash
Created attachment 559732 [details] [diff] [review] Potential ILFindLastID crash fix It doesn't crash for me at all any more even with a release config. I even tried putting the reveal function in a loop of 100,000 times and no crashes. I'm submitting a patch using CoTaskMemFree to see if that helps as mentioned in my Comment 7. I noticed Google Chrome uses CoTaskMemFree. I'll push as soon as possible once I get a review. If it still crashes after that though then I guess I'll back out the original patch and 2 crash fix patches. As I mentioned I can't reproduce a crash so it is a blind fix. I also did a couple small cleanups in the patch as well but the thing that really matters is the CoTaskMemFree.
The array count calculation was also fixed in this patch but even before this patch it was being calculated as 1 item in the array which is the same as now using the proper calculation (x86). That could have maybe been a problem on x64 builds though. > UINT count = sizeof(selection) / sizeof(ITEMIDLIST); Should have been: > UINT count = sizeof(selection) / sizeof(ITEMIDLIST*); But sizeof(ITEMIDLIST) is 3 so it's not a problem for sure on x86 builds.
Just found out that I can reproduce on Release x64 the second crash. So for sure the second crash is for x64 builds and the fix in Comment 22 will fix it.
I had the following crash on the Win64 20110910 Nightly build: https://crash-stats.mozilla.com/report/index/bp-a20a27ca-167e-46a4-8006-b26fc2110910 My Win32 Nightly does not crash following your patch for it.
> I had the following crash on the Win64 20110910 Nightly build: Ya the second crash only affects x64 builds. The first crash affected both. First crash is fixed, second is pending me pushing the new patch to m-c. I ran the patch for the second crash through try and it is passing: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=8107d6e1e854 I'll push directly to m-c if it gets an r+
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/da2f5b63ba1e
Using today's Win64 build (20110913, C-set: da2f5b63ba1e), I was able to open the folder containing a downloaded file and did not crash. Thanks for your hard work.