Closed
Bug 685847
Opened 13 years ago
Closed 13 years ago
Crash in nsLocalFile::RevealUsingShell() on release builds
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: jmjjeffery, Assigned: bbondy)
References
Details
(Keywords: regression)
Crash Data
Attachments
(2 files)
1.22 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
5.26 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
Assignee: nobody → netzen
Assignee | ||
Updated•13 years ago
|
Component: XPCOM → Widget: Win32
QA Contact: xpcom → win32
Updated•13 years ago
|
Crash Signature: https://crash-stats.mozilla.com/report/index/bp-54b73685-d4a7-49b4-8caf-e3d362110909 → [@ nsAString_internal::Finalize() ]
Comment 1•13 years ago
|
||
Another crash ID: https://crash-stats.mozilla.com/report/index/bp-23eeed22-6338-4ce5-ab66-3c2312110909
Assignee | ||
Comment 2•13 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.
Reporter | ||
Comment 3•13 years ago
|
||
Crashes with a new Profile as well, with no addons or history of any sort.
Comment 4•13 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•13 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•13 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•13 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•13 years ago
|
Summary: [@ nsAString_internal::Finalize() ] → Crash in nsLocalFile::RevealUsingShell() on release builds
Assignee | ||
Comment 8•13 years ago
|
||
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•13 years ago
|
||
Any time I load functions dynamically I'll be trying on release builds from now on :)
Assignee | ||
Comment 10•13 years ago
|
||
> was I get a review.
*once I get a review
Comment 11•13 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•13 years ago
|
||
k will do, thanks for the quick review.
Assignee | ||
Comment 13•13 years ago
|
||
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/d078623f7875
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 14•13 years ago
|
||
Verified fixed using latest hourly based on cset: http://hg.mozilla.org/mozilla-central/rev/d078623f7875
Status: RESOLVED → VERIFIED
Comment 15•13 years ago
|
||
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•13 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•13 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.
Assignee | ||
Comment 20•13 years ago
|
||
Re-opening for the second crash
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 21•13 years ago
|
||
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•13 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•13 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•13 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•13 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•13 years ago
|
Attachment #559732 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 26•13 years ago
|
||
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/da2f5b63ba1e
Comment 28•13 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•13 years ago
|
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Target Milestone: --- → mozilla9
You need to log in
before you can comment on or make changes to this bug.
Description
•