nsFileChannel::AsyncOpen throws for nonexistent files

RESOLVED FIXED in mozilla19

Status

()

RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: bzbarsky, Assigned: torisugari)

Tracking

(Blocks: 5 bugs)

Trunk
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

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?
Note that this is causing bugs like bug 269125...
Blocks: 269125
Flags: blocking1.8b3?

Comment 2

14 years ago
Patches accepted, but we're not going to block on it.
Flags: blocking1.8b3? → blocking1.8b3-

Comment 3

14 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
(Reporter)

Updated

13 years ago
Blocks: 250940

Comment 4

13 years ago
-> default owner
Assignee: darin → nobody
QA Contact: benc → networking.file
Blocks: 424880
Blocks: 623719
Blocks: 646214
Blocks: 740865

Updated

7 years ago
Blocks: 621276
No longer blocks: 646214
Blocks: 746880
(Assignee)

Comment 5

7 years ago
Created attachment 619373 [details] [diff] [review]
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.
(Assignee)

Comment 6

7 years ago
Created attachment 619374 [details] [diff] [review]
unit test (xpcshell)
(Assignee)

Updated

7 years ago
Attachment #619373 - Flags: review?(bzbarsky)
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

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

7 years ago
Created attachment 621236 [details] [diff] [review]
unit test (xpcshell) v2

Addressing comment #9.
Attachment #619374 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #621236 - Flags: review?(bzbarsky)
Comment on attachment 621236 [details] [diff] [review]
unit test (xpcshell) v2

r=me
Attachment #621236 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 12

7 years ago
checkin request.
"nsIFileChannel::AsyncOpen should use DEFER_OPEN flag" or something.
Keywords: helpwanted → checkin-needed
Assignee: nobody → torisugari
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff1f86491512
https://hg.mozilla.org/integration/mozilla-inbound/rev/29e94992504b
Flags: blocking1.8b3- → in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla15
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 → ---
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
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

7 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

7 years ago
Created attachment 622851 [details] [diff] [review]
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...
(Assignee)

Updated

7 years ago
Attachment #622851 - Flags: review?(axel)
(Assignee)

Updated

6 years ago
Attachment #622851 - Flags: review?(axel) → review?(l10n)

Comment 20

6 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

6 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)
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

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

Comment 24

6 years ago
Created attachment 674605 [details] [diff] [review]
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.
(Assignee)

Comment 25

6 years ago
And try-server-needed.
Keywords: checkin-needed
This doesn't apply cleanly to mozilla-central. Please rebase and I can push it to Try for you.
Keywords: checkin-needed
What does the actual reftest failure look like?  Link to reftest analyzer for a failing run, please?
(Assignee)

Comment 28

6 years ago
Created attachment 674655 [details] [diff] [review]
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.
Attachment #674605 - Attachment is obsolete: true

Comment 29

6 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.
(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
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.
Nevermind, the Android R2 and R4 failures are not from this. The OSX b-c ones are, though.
(Assignee)

Comment 33

6 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

6 years ago
Created attachment 675037 [details] [diff] [review]
patch v2

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

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b027df4ba1e8
https://hg.mozilla.org/mozilla-central/rev/f818459666b2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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
This seems to have stopped the persona pop-up window on metrics.mozilla.com from working for me on mac.
Hmm.  How is that using a file:// URI?
That part I don't know :)

I just couldn't access metrics, and bisected it to this patch.
That's very very odd.  If you see that problem in safe mode, please file a bug blocking this one with steps to reproduce?

Updated

6 years ago
Duplicate of this bug: 841679
You need to log in before you can comment on or make changes to this bug.