Closed
Bug 282432
Opened 20 years ago
Closed 12 years ago
nsFileChannel::AsyncOpen throws for nonexistent files
Categories
(Core :: Networking: File, defect)
Core
Networking: File
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: bzbarsky, Assigned: torisugari)
References
(Blocks 3 open bugs)
Details
Attachments
(2 files, 5 obsolete files)
2.14 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
4.24 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Right now, nsFileChannel::AsyncOpen throws if the file does not exist. Perhaps,
for consistency with other channel types, we should just return NS_OK and then
report an error in OnStartRequest?
Reporter | ||
Comment 1•20 years ago
|
||
Note that this is causing bugs like bug 269125...
Blocks: 269125
Flags: blocking1.8b3?
Comment 2•19 years ago
|
||
Patches accepted, but we're not going to block on it.
Flags: blocking1.8b3? → blocking1.8b3-
Comment 3•19 years ago
|
||
The solution to this bug probably requires two changes:
* Make nsFileChannel::EnsureStream succeed even if the IsDirectory test fails
* Make nsFileInputStream::Init not call Open. instead call Open lazily from
Read or Available.
Keywords: helpwanted
Updated•13 years ago
|
Assignee | ||
Comment 5•13 years ago
|
||
This seems working as expected. Maybe I should call nsIFile::exists() explicitly rather than check |rv|. In any case, bug 634666 actually fixed this bug. Just last one step.
Assignee | ||
Comment 6•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #619373 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 7•13 years ago
|
||
Comment on attachment 619373 [details] [diff] [review]
patch v1
Ah, nice. r=me, and sorry for the lag!
Attachment #619373 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 8•13 years ago
|
||
Comment on attachment 619374 [details] [diff] [review]
unit test (xpcshell)
I forgot to set the review request for some reason.
Attachment #619374 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 9•13 years ago
|
||
Comment on attachment 619374 [details] [diff] [review]
unit test (xpcshell)
Can you make the test fail if onDataAvailable is actually called?
Also watch out for OSes where you'll get a permissions error for / instead of FILE_NOT_FOUND.
r=me
Attachment #619374 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 10•13 years ago
|
||
Addressing comment #9.
Attachment #619374 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #621236 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 11•13 years ago
|
||
Comment on attachment 621236 [details] [diff] [review]
unit test (xpcshell) v2
r=me
Attachment #621236 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 12•13 years ago
|
||
checkin request.
"nsIFileChannel::AsyncOpen should use DEFER_OPEN flag" or something.
Keywords: helpwanted → checkin-needed
Updated•13 years ago
|
Assignee: nobody → torisugari
Reporter | ||
Comment 13•13 years ago
|
||
The correct checkin comment should be something like:
Calling asyncOpen on a file channel should notify about file not found errors
asynchronously instead of throwing from asyncOpen.
Comment 14•13 years ago
|
||
Comment 15•13 years ago
|
||
Had to back this out due to mochitest orange. Please run any future revisions through Try.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf8b1f792ce
1558 ERROR TEST-UNEXPECTED-FAIL | /tests/security/ssl/mixedcontent/test_bug383369.html | FAILURE: Exception thrown during test: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIRDFService.GetDataSourceBlocking]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: resource:///components/nsHandlerService.js :: <TOP_LEVEL> :: line 932" data: no]
1559 ERROR TEST-UNEXPECTED-FAIL | /tests/security/ssl/mixedcontent/test_bug383369.html | an unexpected uncaught JS exception reported through window.onerror - NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIRDFService.GetDataSourceBlocking] at resource:///components/nsHandlerService.js:932
1560 ERROR TEST-UNEXPECTED-FAIL | /tests/security/ssl/mixedcontent/test_bug383369.html | Test timed out.
Target Milestone: mozilla15 → ---
Comment 16•13 years ago
|
||
And xpcshell failures:
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/uriloader/exthandler/tests/unit/test_handlerService.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>
### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /var/folders/Hs/HsDn6a9SG8idoIya6p9mtE+++TI/-Tmp-/tmpYx_FKQ/runxpcshelltests_leaks.log
*** HandlerServiceTest: getFile: requesting UMimTyp
*** HandlerServiceTest: getFile: requesting CurProcD
*** HandlerServiceTest: the following NS_ERROR_FAILURE exception in nsIDirectoryServiceProvider::getFile is expected, as we don't provide the 'CurProcD' file
TEST-INFO | (xpcshell/head.js) | test 1 pending
*** HandlerServiceTest: getFile: requesting ProfLDS
*** HandlerServiceTest: the following NS_ERROR_FAILURE exception in nsIDirectoryServiceProvider::getFile is expected, as we don't provide the 'ProfLDS' file
JS Component Loader: ERROR (null):0
uncaught exception: 2147500037
*** HandlerServiceTest: getFile: requesting ProfLDS
*** HandlerServiceTest: the following NS_ERROR_FAILURE exception in nsIDirectoryServiceProvider::getFile is expected, as we don't provide the 'ProfLDS' file
JS Component Loader: ERROR (null):0
uncaught exception: 2147500037
*** HandlerServiceTest: getFile: requesting ProfLDS
*** HandlerServiceTest: the following NS_ERROR_FAILURE exception in nsIDirectoryServiceProvider::getFile is expected, as we don't provide the 'ProfLDS' file
JS Component Loader: ERROR (null):0
uncaught exception: 2147500037
*** HandlerServiceTest: getFile: requesting ProfLDS
*** HandlerServiceTest: the following NS_ERROR_FAILURE exception in nsIDirectoryServiceProvider::getFile is expected, as we don't provide the 'ProfLDS' file
JS Component Loader: ERROR (null):0
uncaught exception: 2147500037
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file ../../../../netwerk/base/src/nsFileStreams.cpp, line 338
*** HandlerServiceTest: getFile: requesting TmpD
*** HandlerServiceTest: the following NS_ERROR_FAILURE exception in nsIDirectoryServiceProvider::getFile is expected, as we don't provide the 'TmpD' file
*** HandlerServiceTest: getFile: requesting ProfLDS
*** HandlerServiceTest: the following NS_ERROR_FAILURE exception in nsIDirectoryServiceProvider::getFile is expected, as we don't provide the 'ProfLDS' file
JS Component Loader: ERROR (null):0
uncaught exception: 2147500037
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file ../../../../netwerk/base/src/nsFileStreams.cpp, line 338
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/uriloader/exthandler/tests/unit/test_handlerService.js | 2 == 0 - See following stack:
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 462
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: _do_check_eq :: line 556
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: do_check_eq :: line 577
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/tests/uriloader/exthandler/tests/unit/test_handlerService.js :: run_test :: line 182
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: _execute_test :: line 326
JS frame :: -e :: <TOP_LEVEL> :: line 1
Comment 17•13 years ago
|
||
And reftest failures:
REFTEST TEST-START | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-2.html | 5051 / 7606 (66%)
++DOMWINDOW == 39 (0xaa0bf98) [serial = 13512] [outer = 0x9e177c0]
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file ../../../../netwerk/base/src/nsFileStreams.cpp, line 360
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file ../../../../netwerk/base/src/nsFileStreams.cpp, line 360
REFTEST TEST-START | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-ref-black140x100.html | 5051 / 7606 (66%)
++DOMWINDOW == 40 (0xb40eeb8) [serial = 13513] [outer = 0x9e177c0]
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-2.html | image comparison (==)
Assignee | ||
Comment 18•13 years ago
|
||
Oops.
I'm sorry for these troubles, and thank you.
I ran these tests and I couldn't reproduce mochitest's and reftest's. Both were green. Probably either platform specific, or race condition. I could surely reproduce that xpcshell's.
Assignee | ||
Comment 19•13 years ago
|
||
This bug is going to suppress nsBaseChannel::OpenContentStream()'s error. i.e. It affects not only nsIChannel::asyncOpen() but also nsIChannel::open(). As a result, the trick like
> rv = channel->Open(getter_AddRefs(in));
>-
>- // Report success if the file doesn't exist, but propagate other errors.
>- if (rv == NS_ERROR_FILE_NOT_FOUND) return NS_OK;
stop working.
On the other hand, I'm at a loss how to handle <video poster="...">.
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/ogg-video/poster-2.html?force=1
Maybe this is a race-condition issue, but erroneous poster detection should work asynchronously. If synchronous network error is required for some reason, proabably we can use PROTOCOL_NOT_FOUND instead of FILE_NOT_FOUND, for example, <video poster="foo:bar">. At any rate, since I can't reproduce the reftest failure, I'm not too sure that can be a proper workaround...
Assignee | ||
Updated•13 years ago
|
Attachment #622851 -
Flags: review?(axel)
Assignee | ||
Updated•12 years ago
|
Attachment #622851 -
Flags: review?(axel) → review?(l10n)
Comment 20•12 years ago
|
||
Comment on attachment 622851 [details] [diff] [review]
fix orange v1 (RDFDataSource)
Review of attachment 622851 [details] [diff] [review]:
-----------------------------------------------------------------
I'm sceptical, I guess I'd rather have the file-not-found be caught where it's actually raised, but I wouldn't know where.
I'd rather have this reviewed by someone that's actually close to the networking bits, and I don't insist on a rdf-side review on this one.
Clearing review as I won't get time to say anything decisive either way soon.
Attachment #622851 -
Flags: review?(l10n)
Assignee | ||
Comment 21•12 years ago
|
||
Comment on attachment 622851 [details] [diff] [review]
fix orange v1 (RDFDataSource)
(In reply to Axel Hecht from comment #20)
> I'm sceptical, I guess I'd rather have the file-not-found be caught where
> it's actually raised, but I wouldn't know where.
That's probably here:
> channel->GetStatus(&rv);
http://hg.mozilla.org/mozilla-central/annotate/1c3e4cb1f754/rdf/base/src/nsRDFXMLDataSource.cpp#l525
However, once BlockingParse() calls OnBeginLoad(...) and onStartRequest(...), it should not return NS_ERROR_FILE_NOT_FOUND (or any value) before calling onStopRequest() and OnEndLoad(...). That's why I think we should wait until BlockingParse() successfully returns an error code.
By the way, in my ultimately honest opinion, nIRDFDataSource::refresh() is a crazy API method, which accepts a FILE_NOT_FOUND URI and refuses a URI pointing to an empty file.
Attachment #622851 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 22•12 years ago
|
||
Comment on attachment 622851 [details] [diff] [review]
fix orange v1 (RDFDataSource)
This seems fine to me.
Attachment #622851 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 23•12 years ago
|
||
Thanks. I'll try to reproduce the reftest error (comment #17) again.
Assignee | ||
Comment 24•12 years ago
|
||
(In reply to O. Atsushi (Torisugari) from comment #23)
> I'll try to reproduce the reftest error (comment #17) again.
And I'm at a loss what to do.
my log:
++DOMWINDOW == 29 (0x48ccae6c) [serial = 58] [outer = 0x4056ad90]
REFTEST TEST-START | file:///home/atsushi/dev/mozilla/src-tmp/layout/reftests/ogg-video/poster-2.html | 20 / 32 (62%)
++DOMWINDOW == 30 (0x48cceddc) [serial = 59] [outer = 0x4056ad90]
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /home/atsushi/dev/mozilla/src-tmp/netwerk/base/src/nsFileStreams.cpp, line 167
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /home/atsushi/dev/mozilla/src-tmp/netwerk/base/src/nsFileStreams.cpp, line 439
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /home/atsushi/dev/mozilla/src-tmp/netwerk/base/src/nsFileStreams.cpp, line 167
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /home/atsushi/dev/mozilla/src-tmp/netwerk/base/src/nsFileStreams.cpp, line 439
REFTEST TEST-START | file:///home/atsushi/dev/mozilla/src-tmp/layout/reftests/ogg-video/poster-ref-black140x100.html | 20 / 32 (62%)
++DOMWINDOW == 31 (0x48ccbebc) [serial = 60] [outer = 0x4056ad90]
REFTEST TEST-PASS | file:///home/atsushi/dev/mozilla/src-tmp/layout/reftests/ogg-video/poster-2.html | image comparison (==)
This test fetches data from a not-found file URI intentionally. Then maybe nsFileStreams.cpp should return the error in this way:
if (NS_FAILED(rv))
return rv;
instead of NS_ENSURE_SUCCESS() macro.
http://hg.mozilla.org/mozilla-central/annotate/1c3e4cb1f754/netwerk/base/src/nsFileStreams.cpp#l167
http://hg.mozilla.org/mozilla-central/annotate/1c3e4cb1f754/netwerk/base/src/nsFileStreams.cpp#l439
But that does not explain "REFTEST TEST-UNEXPECTED-FAIL" in comment #17.
Comment 26•12 years ago
|
||
This doesn't apply cleanly to mozilla-central. Please rebase and I can push it to Try for you.
Keywords: checkin-needed
Reporter | ||
Comment 27•12 years ago
|
||
What does the actual reftest failure look like? Link to reftest analyzer for a failing run, please?
Assignee | ||
Comment 28•12 years ago
|
||
(In reply to Ryan VanderMeulen from comment #26)
> This doesn't apply cleanly to mozilla-central. Please rebase and I can push
> it to Try for you.
I'm really sorry, and thank you.
(In reply to Boris Zbarsky (:bz) from comment #27)
> What does the actual reftest failure look like? Link to reftest analyzer
> for a failing run, please?
My build always passes the test, in my environment. All what I know is comment #17.
Attachment #674605 -
Attachment is obsolete: true
Comment 29•12 years ago
|
||
I was wondering, how does async opening of RDF files deal with non-existing files? Sorry, on PTO, thus not spending a lot of time on this this week.
Comment 30•12 years ago
|
||
(In reply to O. Atsushi (Torisugari) from comment #28)
> I'm really sorry, and thank you.
Not a problem!
https://tbpl.mozilla.org/?tree=Try&rev=fbc239e213ce
Comment 31•12 years ago
|
||
It was a pretty rough day for the tree, but I would say that the OSX b-c failures and the Android R4 failures are caused by this patch. Maybe the Android R2 as well. The rest is just flaky tests.
Comment 32•12 years ago
|
||
Nevermind, the Android R2 and R4 failures are not from this. The OSX b-c ones are, though.
Assignee | ||
Comment 33•12 years ago
|
||
> The OSX b-c ones are, though.
It seems time to start blind hacking.
The tests listed in the error log does not seems to use loadSubScript(). i.e. Mochitest is calling loadSubScript(). So what's FILE_NOT_FOUND are test scripts. That implies OSX sometimes fails to copy (or create shortcut to) test script files...?
Anyway, mozJSSubScriptLoader::LoadSubScript(...) reads a file synchronously.
http://hg.mozilla.org/mozilla-central/annotate/1c3e4cb1f754/js/xpconnect/loader/mozJSSubScriptLoader.cpp#l88
And what I should do is pass NS_ERROR_FILE_NOT_FOUND to cx through ReportError(), instead of return NS_ERROR_FILE_NOT_FOUND.
Possible solutions would be
1. nsIChannel::open(...) returns NS_ERROR_FILE_NOT_FOUND
while nsIChannel::asyncOpen(...) returns NS_OK.
2. Set -1 to contentLength, instead of 0, on NOT_FOUND.
3. Make loadSubScript() pass LOAD_ERROR_NOCONTENT to cx when
contentLength is 0 besides 1.
That is,
> int64_t len = -1;
>
> rv = chan->GetContentLength(&len);
> if (NS_FAILED(rv) || len == -1) {
> return ReportError(cx, LOAD_ERROR_NOCONTENT);
> }
s/"len == -1"/"len <= 0"/.
I think 1. is the most conservative plan though such a difference between open() and asyncOpen() may cause something troublesome in the future.
Assignee | ||
Comment 34•12 years ago
|
||
Someone OSX user should take this bug, no?
Attachment #619373 -
Attachment is obsolete: true
Attachment #622851 -
Attachment is obsolete: true
Attachment #674655 -
Attachment is obsolete: true
Attachment #675037 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 35•12 years ago
|
||
Comment on attachment 675037 [details] [diff] [review]
patch v2
Yeah, this seems reasonable. Throwing from open() is perfectly acceptable.
Attachment #675037 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 36•12 years ago
|
||
Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=079dc1ebac54
Comment 37•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b027df4ba1e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f818459666b2
Keywords: checkin-needed
Comment 38•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b027df4ba1e8
https://hg.mozilla.org/mozilla-central/rev/f818459666b2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 39•12 years ago
|
||
I just bisected and found that the first patch (b027df4ba1e8) caused the Chatzilla and Tree Style Tab extensions to not work: bug 810059.
Depends on: 810059
Comment 40•12 years ago
|
||
This seems to have stopped the persona pop-up window on metrics.mozilla.com from working for me on mac.
Reporter | ||
Comment 41•12 years ago
|
||
Hmm. How is that using a file:// URI?
Comment 42•12 years ago
|
||
That part I don't know :)
I just couldn't access metrics, and bisected it to this patch.
Reporter | ||
Comment 43•12 years ago
|
||
That's very very odd. If you see that problem in safe mode, please file a bug blocking this one with steps to reproduce?
You need to log in
before you can comment on or make changes to this bug.
Description
•