Last Comment Bug 713485 - Crash when invalid drive is used in local file URL
: Crash when invalid drive is used in local file URL
Status: RESOLVED FIXED
: crash
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows 7
: -- critical with 1 vote (vote)
: mozilla13
Assigned To: Brian R. Bondy [:bbondy]
:
: Jim Mathies [:jimm]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-25 17:53 PST by Atte Kettunen
Modified: 2012-02-09 10:27 PST (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
634604657471421591.html (89.79 KB, text/html)
2011-12-25 17:53 PST, Atte Kettunen
no flags Details
sample html (31 bytes, text/html)
2011-12-26 12:24 PST, Alice0775 White
no flags Details
Patch v1. (2.79 KB, patch)
2012-01-31 20:05 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch v2. (2.12 KB, patch)
2012-02-06 06:52 PST, Brian R. Bondy [:bbondy]
neil: review+
Details | Diff | Splinter Review

Description Atte Kettunen 2011-12-25 17:53:36 PST
Created attachment 584297 [details]
634604657471421591.html

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:12.0a1) Gecko/20111224 Firefox/12.0a1
Build ID: 20111224031026

Steps to reproduce:

Open attachment file in Nightly build firefox. I'll reduce the repro-case after the Holidays.


Actual results:

Crash
Comment 1 Matthias Versen [:Matti] 2011-12-25 20:41:22 PST
I can confirm a crash if I load the testcase via local filesystem.

This seems to be the regression range:
Last good nightly: 2011-09-21
First bad nightly: 2011-09-22

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e8bd19f6abbb&tochan
ge=4495e1f795c2
Comment 2 Matthias Versen [:Matti] 2011-12-25 21:07:05 PST
The crash report is useless: bp-f0aa99af-c05c-4735-9e50-52ced2111225 except that it confirms EXCEPTION_NONCONTINUABLE_EXCEPTION
Comment 3 Alice0775 White 2011-12-26 01:58:13 PST
Regression window(m-c):
Works:
http://hg.mozilla.org/mozilla-central/rev/e8bd19f6abbb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110921 Firefox/9.0a1 ID:20110921030906
Fails:
http://hg.mozilla.org/mozilla-central/rev/3178f1c42505
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110921 Firefox/9.0a1 ID:20110921013649
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e8bd19f6abbb&tochange=3178f1c42505


Regression window(m-c):
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1e41259daf67
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110920 Firefox/9.0a1 ID:20110920121748
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6bcc3a001bb3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110920 Firefox/9.0a1 ID:20110920140549
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1e41259daf67&tochange=6bcc3a001bb3

Triggered by:
c237a8550070	Boris Zbarsky — Bug 444641 part 3. Propagate the loading principal to the image channel as needed. r=joe,dveditz
Comment 4 Alice0775 White 2011-12-26 02:15:12 PST
bp-fd7eaf2f-3039-405c-9cdd-8b2bb2111226
Comment 5 Atte Kettunen 2011-12-26 04:52:04 PST
Reduced test-case is file with the following content:

<BODY background="/$:"/>

Results:
https://crash-stats.mozilla.com/report/index/bp-141fb79b-cfd9-4c64-a5ba-668502111226
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-12-26 08:30:20 PST
I can't reproduce the crash in a trunk build on Mac.  Is this Windows-only?

The crash report stack is obviously busted, sadly.  :(  But I can buy us crashing on a null-deref here.

Alice0775, do you happen to have a debug build by any chance?  Accurate information on where this is actually crashing would be really useful!
Comment 7 Atte Kettunen 2011-12-26 08:42:54 PST
Can't reproduce on Ubuntu 11.04 x64 either. Seems like windows only. Can someone clarify the noncontinuable exception for me? Never faced one before.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-12-26 08:54:50 PST
I don't think it's worth worrying about the exception type.  Given the crash address, this is a null-deref; given the Windows-only thing and the testcase, it's somewhere in nsLocalFileWin.  The question is where....
Comment 9 Alice0775 White 2011-12-26 12:24:15 PST
Created attachment 584340 [details]
sample html

bp-461a2f5b-b2d1-45cf-b0f3-007322111226
bp-5159f007-b54a-4846-b596-5b3342111226

$: also crashes on Firefox3.0-12.0
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-12-26 12:39:59 PST
Those stacks are still showing crashes at points where there is nothing there to crash.  :(
Comment 11 Masatoshi Kimura [:emk] 2011-12-26 19:38:37 PST
> int drive = TOUPPER(path.First()) - 'A' + 1;
Obviously |drive| will have a bogus value when the drive letter is invalid.
Comment 12 Brian R. Bondy [:bbondy] 2012-01-31 20:05:12 PST
Created attachment 593302 [details] [diff] [review]
Patch v1.
Comment 13 neil@parkwaycc.co.uk 2012-02-01 13:45:02 PST
Indeed, running the test case in a debug build is much more effective:

Microsoft Visual C++ Debug Library

Debug Assertion Failed!

Program: ...
File: ...\crt\src\getcwd.c
Line: 233

Expression: ("Invalid Drive", 0)

Sadly Gecko then hangs because it executes a posted runnable that tries to XHR to perform an addon check that tries to create the bad cert handler for the first time by loading it from the startup cache even though I used -purgecaches which deadlocks when the startup cache thread tries to access the CRT...
Comment 14 neil@parkwaycc.co.uk 2012-02-01 14:22:54 PST
Actually what worries me is why we are hitting this code at all because file:///C:/ should be a canonical path...
Comment 15 Brian R. Bondy [:bbondy] 2012-02-01 16:03:21 PST
At this point in code we're no longer working with file URLs but actual file paths.

nsPrincipal::CheckMayLoad calls fileURL->GetFile() to get the targetFile, and then calls Normalize on that target file.
Comment 16 neil@parkwaycc.co.uk 2012-02-02 13:14:48 PST
Yes, I do understand that Normalize has been called, but C:\ is already canonical, so we shouldn't be trying to canonicalise it in this case.

The easiest example I could find of this is that launching composer using seamonkey -edit C:\ actually loads the listing for the current directory!
Comment 17 Brian R. Bondy [:bbondy] 2012-02-02 13:29:39 PST
I understand your concern and I'd like to investigate that in the context of another bug since it isn't related to this crash.  For example this crash can be reproduced by anyone using nsIFile and so this fix should be applied independently.  

Please let me know if you disagree.
Comment 18 neil@parkwaycc.co.uk 2012-02-05 13:30:28 PST
Comment on attachment 593302 [details] [diff] [review]
Patch v1.

In that case I would prefer it if InitWithPath failed immediately.
Comment 19 Brian R. Bondy [:bbondy] 2012-02-06 06:52:29 PST
Created attachment 594698 [details] [diff] [review]
Patch v2.

Thanks for the review.  Implemented review comments.
Comment 20 Brian R. Bondy [:bbondy] 2012-02-06 10:41:56 PST
In case it gets an r+, this is passing try.
Comment 22 Ed Morley [:emorley] 2012-02-09 10:27:22 PST
https://hg.mozilla.org/mozilla-central/rev/094e4a9472a1

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