Last Comment Bug 685847 - Crash in nsLocalFile::RevealUsingShell() on release builds
: Crash in nsLocalFile::RevealUsingShell() on release builds
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86_64 Windows 7
: -- critical with 1 vote (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
: 686177 686302 (view as bug list)
Depends on:
Blocks: 670068 716449
  Show dependency treegraph
 
Reported: 2011-09-09 06:21 PDT by Jim Jeffery not reading bug-mail 1/2/11
Modified: 2012-01-19 03:02 PST (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fixed crash by specifying WINAPI on typedef (1.22 KB, patch)
2011-09-09 11:41 PDT, Brian R. Bondy [:bbondy]
jmathies: review+
Details | Diff | Splinter Review
Potential ILFindLastID crash fix (5.26 KB, patch)
2011-09-10 20:37 PDT, Brian R. Bondy [:bbondy]
jmathies: review+
Details | Diff | Splinter Review

Description Jim Jeffery not reading bug-mail 1/2/11 2011-09-09 06:21:57 PDT
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
Comment 2 Brian R. Bondy [:bbondy] 2011-09-09 06:38:30 PDT
Strangely enough this crash does not happen on my compiled build, nor on the Nightly Debug build, but does on the Nightly release build.
Comment 3 Jim Jeffery not reading bug-mail 1/2/11 2011-09-09 06:59:41 PDT
Crashes with a new Profile as well, with no addons or history of any sort.
Comment 4 WildcatRay 2011-09-09 07:03:26 PDT
FWIW, the 64-bit build locked up as reported by the Win7 program has stopped running dialog, but no crash.
Comment 5 Brian R. Bondy [:bbondy] 2011-09-09 07:05:00 PDT
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.
Comment 6 Brian R. Bondy [:bbondy] 2011-09-09 07:18:56 PDT
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.
Comment 7 Brian R. Bondy [:bbondy] 2011-09-09 09:42:13 PDT
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.
Comment 8 Brian R. Bondy [:bbondy] 2011-09-09 11:41:07 PDT
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.
Comment 9 Brian R. Bondy [:bbondy] 2011-09-09 11:42:01 PDT
Any time I load functions dynamically I'll be trying on release builds from now on :)
Comment 10 Brian R. Bondy [:bbondy] 2011-09-09 11:42:47 PDT
> was I get a review.
*once I get a review
Comment 11 Jim Mathies [:jimm] 2011-09-09 11:53:16 PDT
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.
Comment 12 Brian R. Bondy [:bbondy] 2011-09-09 11:56:21 PDT
k will do, thanks for the quick review.
Comment 13 Brian R. Bondy [:bbondy] 2011-09-09 12:04:07 PDT
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/d078623f7875
Comment 14 Jim Jeffery not reading bug-mail 1/2/11 2011-09-09 15:12:46 PDT
Verified fixed using latest hourly based on cset:

http://hg.mozilla.org/mozilla-central/rev/d078623f7875
Comment 15 Marcia Knous [:marcia - use ni] 2011-09-09 16:03:42 PDT
Adding [@ ILFindLastID ] since it appears to be related to this bug and is showing up in crash stats as a new crash.
Comment 16 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-09-10 11:20:59 PDT
(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.
Comment 17 Brian R. Bondy [:bbondy] 2011-09-10 15:24:12 PDT
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?
Comment 18 WildcatRay 2011-09-10 15:59:57 PDT
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.
Comment 19 gabe 2011-09-10 19:00:34 PDT
*** Bug 686177 has been marked as a duplicate of this bug. ***
Comment 20 Brian R. Bondy [:bbondy] 2011-09-10 20:09:29 PDT
Re-opening for the second crash
Comment 21 Brian R. Bondy [:bbondy] 2011-09-10 20:37:10 PDT
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.
Comment 22 Brian R. Bondy [:bbondy] 2011-09-10 21:18:34 PDT
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.
Comment 23 Brian R. Bondy [:bbondy] 2011-09-10 21:23:38 PDT
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.
Comment 24 WildcatRay 2011-09-11 05:31:25 PDT
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.
Comment 25 Brian R. Bondy [:bbondy] 2011-09-11 07:38:58 PDT
> 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+
Comment 26 Brian R. Bondy [:bbondy] 2011-09-12 07:28:27 PDT
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/da2f5b63ba1e
Comment 27 Jim Jeffery not reading bug-mail 1/2/11 2011-09-12 09:04:00 PDT
*** Bug 686302 has been marked as a duplicate of this bug. ***
Comment 28 WildcatRay 2011-09-13 07:51:41 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.