685847 - Crash in nsLocalFile::RevealUsingShell() on release builds

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

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 1

[WildcatRay]

Another crash ID: https://crash-stats.mozilla.com/report/index/bp-23eeed22-6338-4ce5-ab66-3c2312110909

Comment 2

[Brian R. Bondy [:bbondy]]

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]

Crashes with a new Profile as well, with no addons or history of any sort.

Comment 4

[WildcatRay]

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

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

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

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

Attachment: 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]]

Any time I load functions dynamically I'll be trying on release builds from now on :)

Comment 10

[Brian R. Bondy [:bbondy]]

> was I get a review.
*once I get a review

Comment 11

[Jim Mathies [:jimm]]

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

k will do, thanks for the quick review.

Comment 13

[Brian R. Bondy [:bbondy]]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/d078623f7875

Comment 14

[Jim Jeffery not reading bug-mail 1/2/11]

Verified fixed using latest hourly based on cset:

http://hg.mozilla.org/mozilla-central/rev/d078623f7875

Comment 15

[Marcia Knous [:marcia]]

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-4, no longer working on Mozilla)]

(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]]

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]

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 20

[Brian R. Bondy [:bbondy]]

Re-opening for the second crash

Comment 21

[Brian R. Bondy [:bbondy]]

Attachment: 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]]

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

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]

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

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

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/da2f5b63ba1e

Comment 28

[WildcatRay]

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.