282432 - nsFileChannel::AsyncOpen throws for nonexistent files

Status: RESOLVED FIXED

(Core :: Networking: File, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that this is causing bugs like bug 269125...

Comment 2

[Benjamin Smedberg]

Patches accepted, but we're not going to block on it.

Comment 3

[Darin Fisher]

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.

Comment 4

[Darin Fisher]

-> default owner

Comment 5

[O. Atsushi (Torisugari)]

Attachment: patch v1

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.

Comment 6

[O. Atsushi (Torisugari)]

Attachment: unit test (xpcshell)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 619373 [details] [diff] [review]
patch v1

Ah, nice.  r=me, and sorry for the lag!

Comment 8

[O. Atsushi (Torisugari)]

Comment on attachment 619374 [details] [diff] [review]
unit test (xpcshell)

I forgot to set the review request for some reason.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[O. Atsushi (Torisugari)]

Attachment: unit test (xpcshell) v2

Addressing comment #9.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 621236 [details] [diff] [review]
unit test (xpcshell) v2

r=me

Comment 12

[O. Atsushi (Torisugari)]

checkin request.
"nsIFileChannel::AsyncOpen should use DEFER_OPEN flag" or something.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff1f86491512
https://hg.mozilla.org/integration/mozilla-inbound/rev/29e94992504b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[O. Atsushi (Torisugari)]

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.

Comment 19

[O. Atsushi (Torisugari)]

Attachment: fix orange v1 (RDFDataSource)

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

Comment 20

[Axel Hecht]

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.

Comment 21

[O. Atsushi (Torisugari)]

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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 622851 [details] [diff] [review]
fix orange v1 (RDFDataSource)

This seems fine to me.

Comment 23

[O. Atsushi (Torisugari)]

Thanks. I'll try to reproduce the reftest error (comment #17) again.

Comment 24

[O. Atsushi (Torisugari)]

Attachment: all-in-one for checkin (s/nsnull/nullptr/)

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

[O. Atsushi (Torisugari)]

And try-server-needed.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply cleanly to mozilla-central. Please rebase and I can push it to Try for you.

Comment 27

[Boris Zbarsky [:bzbarsky]]

What does the actual reftest failure look like?  Link to reftest analyzer for a failing run, please?

Comment 28

[O. Atsushi (Torisugari)]

Attachment: all-in-one for checkin v2

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

Comment 29

[Axel Hecht]

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

[Ryan VanderMeulen [:RyanVM]]

(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

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

Nevermind, the Android R2 and R4 failures are not from this. The OSX b-c ones are, though.

Comment 33

[O. Atsushi (Torisugari)]

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

Comment 34

[O. Atsushi (Torisugari)]

Attachment: patch v2

Someone OSX user should take this bug, no?

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 675037 [details] [diff] [review]
patch v2

Yeah, this seems reasonable.  Throwing from open() is perfectly acceptable.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=079dc1ebac54

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b027df4ba1e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f818459666b2

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b027df4ba1e8
https://hg.mozilla.org/mozilla-central/rev/f818459666b2

Comment 39

[Nicholas Nethercote [inactive]]

I just bisected and found that the first patch (b027df4ba1e8) caused the Chatzilla and Tree Style Tab extensions to not work:  bug 810059.

Comment 40

[Matt Woodrow (:mattwoodrow)]

This seems to have stopped the persona pop-up window on metrics.mozilla.com from working for me on mac.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Hmm.  How is that using a file:// URI?

Comment 42

[Matt Woodrow (:mattwoodrow)]

That part I don't know :)

I just couldn't access metrics, and bisected it to this patch.

Comment 43

[Boris Zbarsky [:bzbarsky]]

That's very very odd.  If you see that problem in safe mode, please file a bug blocking this one with steps to reproduce?