858723 - [OS.File] File 0 will not work

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

In the async version of OS.File, osfile_async_worker.js gives a unique number to each file, used to communicate with osfile_async_front.jsm.

In osfile_async_front.jsm, however, we consistently check |if (filenum)| or |if (!filenum)| to ensure that |filenum != null|. This doesn't work if |filenum == 0|.

I have no idea why no test ever caught his, but we need to fix this quickly.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Actually, this seems a little less widespread than I thought.
The first file we open (and only the first file):
- cannot be closed;
- cannot be stat()-ed.

Still, we need to fix this.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing File 0

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 734024 [details] [diff] [review]
Fixing File 0

Review of attachment 734024 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/tests/xpcshell/xpcshell.ini
@@ +1,5 @@
>  [DEFAULT]
>  head =
>  tail =
>  
> +[test_osfile_closed.js]

|hg add| is your friend.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing File 0, v2

Oops

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 735122 [details] [diff] [review]
Fixing File 0, v2

Review of attachment 735122 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/osfile_async_worker.js
@@ +42,5 @@
>             options = data.args[data.args.length - 1];
>           }
>           // If |outExecutionDuration| option was supplied, start measuring the
>           // duration of the operation.
> +         if (options && typeof options === "object" && "outExecutionDuration" in options) {

Is this hunk supposed to be here?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

I confess that it is a drive-by fix. I can split this 1-liner fix into another patch, if you prefer.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> I confess that it is a drive-by fix. I can split this 1-liner fix into
> another patch, if you prefer.

Sneaky!  I think it would be better to split it out into another patch (another bug, even).

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing File 0, v3

Same patch, without the drive-by fix.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

try: https://tbpl.mozilla.org/?tree=Try&rev=a18865728938

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply cleanly to inbound. Please post an updated patch.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing File 0, v4

I haven't experienced any bitrot on inbound. Re-attaching the patch in case I had solved the issue already for some reason.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e27a2e5113

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-other failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bce3c7a0f9b

https://tbpl.mozilla.org/php/getParsedLog.php?id=21913722&tree=Mozilla-Inbound

07:38:37     INFO -  14827 INFO TEST-PASS | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_async.xul | system_shutdown: File descriptors leaks are logged correctly.
07:38:37     INFO -  14828 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_async.xul | system_shutdown: Leaked resource is correctly listed in the log.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

This had xpcshell failures too.
https://tbpl.mozilla.org/php/getParsedLog.php?id=21916880&tree=Mozilla-Inbound

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing File 0, v5

Ok, I believe that I have fixed the issue, which was actually due to another patch that landed in the meantime.
Try: https://tbpl.mozilla.org/?tree=Try&rev=cbf6e1f8bc10

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing File 0, v6

Same one, more testsuite fix.
Try: https://tbpl.mozilla.org/?tree=Try&rev=544c59b0d028

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40a9300f1200

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/40a9300f1200