Crash in nsLocalFile::RevealUsingShell() on release builds

RESOLVED FIXED in mozilla9

Status

()

Core
Widget: Win32
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Jim Jeffery not reading bug-mail 1/2/11, Assigned: bbondy)

Tracking

({regression})

Trunk
mozilla9
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments)

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
(Assignee)

Updated

6 years ago
Assignee: nobody → netzen
(Assignee)

Updated

6 years ago
Component: XPCOM → Widget: Win32
QA Contact: xpcom → win32

Updated

6 years ago
Crash Signature: https://crash-stats.mozilla.com/report/index/bp-54b73685-d4a7-49b4-8caf-e3d362110909 → [@ nsAString_internal::Finalize() ]

Comment 1

6 years ago
Another crash ID: https://crash-stats.mozilla.com/report/index/bp-23eeed22-6338-4ce5-ab66-3c2312110909
(Assignee)

Comment 2

6 years ago
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.

Comment 4

6 years ago
FWIW, the 64-bit build locked up as reported by the Win7 program has stopped running dialog, but no crash.
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Updated

6 years ago
Summary: [@ nsAString_internal::Finalize() ] → Crash in nsLocalFile::RevealUsingShell() on release builds
(Assignee)

Comment 8

6 years ago
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.
Attachment #559527 - Flags: review?(jmathies)
(Assignee)

Comment 9

6 years ago
Any time I load functions dynamically I'll be trying on release builds from now on :)
(Assignee)

Comment 10

6 years ago
> was I get a review.
*once I get a review

Comment 11

6 years ago
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.
Attachment #559527 - Flags: review?(jmathies) → review+
(Assignee)

Comment 12

6 years ago
k will do, thanks for the quick review.
(Assignee)

Comment 13

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

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Verified fixed using latest hourly based on cset:

http://hg.mozilla.org/mozilla-central/rev/d078623f7875
Status: RESOLVED → VERIFIED
Adding [@ ILFindLastID ] since it appears to be related to this bug and is showing up in crash stats as a new crash.
Crash Signature: [@ nsAString_internal::Finalize() ] → [@ nsAString_internal::Finalize() ] [@ ILFindLastID ]
(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.
(Assignee)

Comment 17

6 years ago
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

6 years ago
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.

Updated

6 years ago
Duplicate of this bug: 686177
(Assignee)

Comment 20

6 years ago
Re-opening for the second crash
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

6 years ago
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.
Attachment #559732 - Flags: review?(jmathies)
(Assignee)

Comment 22

6 years ago
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.
(Assignee)

Comment 23

6 years ago
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

6 years ago
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.
(Assignee)

Comment 25

6 years ago
> 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+

Updated

6 years ago
Attachment #559732 - Flags: review?(jmathies) → review+
(Assignee)

Comment 26

6 years ago
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/da2f5b63ba1e
Duplicate of this bug: 686302

Comment 28

6 years ago
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.
(Assignee)

Updated

6 years ago
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Blocks: 716449
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.