793435 - [OS.File] fd.dispose is not a function

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

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

Under Windows, attempting to close an INVALID_HANDLE_VALUE results in the following error: « fd.dispose is not a function ».

We should turn this into a noop.

Comment 1

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

This would make a nice and easy mentored bug. If anybody is interested in picking this up, know that you should modify two functions in file toolkit/components/osfile/osfile_win_back.jsm: |CloseHandle| and |FindClose|.

Comment 2

[WangJun]

Can I take this one, sir?
Only turn it into noop and run the test is enough?

Comment 3

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

With pleasure.
So, what you should do is:
- if the argument is equal to INVALID_HANDLE_VALUE, do nothing;
- otherwise, call method |dispose| as is done currently.

Comment 4

[WangJun]

How to test it?
(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> With pleasure.
So, what you should do is:
- if the argument is equal to
> INVALID_HANDLE_VALUE, do nothing;
- otherwise, call method |dispose| as is
> done currently.

Comment 5

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

From your object directory, after compilation:
  TEST_PATH=toolkit/components/osfile/tests/mochi make mochitest-chrome

Comment 6

[WangJun]

Attachment: patch

Is it the right one ?

Comment 7

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

You should submit your changes as a patch so that I can review them.
For more information about patches and how to make them, you should take a look at:
- https://developer.mozilla.org/en-US/docs/Mercurial
- https://developer.mozilla.org/en-US/docs/Mercurial_Queues

Comment 8

[WangJun]

Attachment: simple patch

Sir, is it like this?

Comment 9

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

Comment on attachment 665750 [details] [diff] [review]
simple patch

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

This is better, but not quite. If you click "splinter review" on the patch, you will notice that the original version (on the left) is not the version present in mozilla-central, but probably one of your intermediary saves.

Comment 10

[WangJun]

Attachment: patch

Comment 11

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

Comment on attachment 666143 [details] [diff] [review]
patch

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

Looks almost good. Please fix the following remarks.
Also, I have realized that you should also add a test.

In file toolkit/components/osfile/tests/mochi/worker_test_osfile_win.js, in function |test_OpenClose|, you should:
- add a call to |OS.Win.File.CloseHandle(OS.Constants.Win.INVALID_HANDLE)|;
- add a call to |OS.Win.File.FindClose(OS.Constants.Win.INVALID_HANDLE)|;
- ensure that both calls return |true|.

::: toolkit/components/osfile/osfile_win_back.jsm
@@ +196,1 @@
>         };

Mostly good. However:
- you should return |true|, not INVALID_HANDLE (or INVILID_HANDLE);
- add spaces around "else".

@@ +204,5 @@
> +         if (Types.int.cast(handle).value == INVALID_HANDLE){
> +           return INVILID_HANDLE;
> +         }else{
> +           return handle.dispose(); // Returns the value of |FindClose|.
> +         }

as above

Comment 12

[WangJun]

Attachment: the patch for bug

Comment 13

[WangJun]

Attachment: for the test

Comment 14

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

WangJun, just a quick note: when you put new patches as replacements for old patches, could you please mark the old patches as obsolete? This makes the life of reviewers much easier.

Comment 15

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

Comment on attachment 666209 [details] [diff] [review]
for the test

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

Looks good. You have my r+ if tests pass.

Comment 16

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

Comment on attachment 666208 [details] [diff] [review]
the patch for bug

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

Looks good. You have my r+ if tests pass.

Comment 17

[WangJun]

Get it. Thank you very much.

(In reply to David Rajchenbach Teller [:Yoric] from comment #14)
> WangJun, just a quick note: when you put new patches as replacements for old
> patches, could you please mark the old patches as obsolete? This makes the
> life of reviewers much easier.

Comment 18

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

Final steps before the bug is finished:
- re-upload the patches with a complete title, including the bug number and the shortname of the reviewer (e.g. "Bug 793435 - CloseHandle and FindClose now handle INVALID_HANDLE correctly;r=yoric");
- mark the bug as "checkin-needed" (add this in the field "keywords" of the page).


We will then land your bug on the repository.

Comment 19

[WangJun]

Attachment: patch for bug 793435

Comment 20

[WangJun]

Attachment: patch for bug

Comment 21

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

Comment on attachment 666283 [details] [diff] [review]
patch for bug

We are almost there.
Two things:
- please don't forget to mark the old patch as obsolete;
- use *keyword* checkin-needed (near the top of the page).

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This is failing tests on Try.
https://tbpl.mozilla.org/?tree=Try&rev=2e9b7dfe0607

13137 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_async.xul | iter: Uncaught error [object WorkerErrorEvent]
13156 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_back.xul | an unexpected uncaught JS exception reported through window.onerror - first argument must be a CData at resource://gre/modules/osfile/osfile_shared_allthreads.jsm:225
13766 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_front.xul | Error: first argument must be a CData

Comment 23

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

WangJun, you should have tested your code!
(and I should have spotted the error)

This error message stages that there is a problem with a |cast|. Here, as it turns out, the cast is not necessary. You can just compare |fd| and |INVALID_HANDLE_VALUE|.

Comment 24

[WangJun]

(In reply to David Rajchenbach Teller [:Yoric] from comment #23)
> WangJun, you should have tested your code!
> (and I should have spotted the error)
> 
> This error message stages that there is a problem with a |cast|. Here, as it
> turns out, the cast is not necessary. You can just compare |fd| and
> |INVALID_HANDLE_VALUE|.

Sorry for the error. Therefore I should run the patch on the Try Server right? Local mochi test is not enough.

Comment 25

[WangJun]

Attachment: Based on the original version

Comment 26

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

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

Note: after discussion on IRC, WangJun's problem was that he was testing the code on Linux, while modifying a Windows-specific module.

Comment 27

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

WangJun: The test has failed (see link above).
I have isolated the problem to the two lines you have added to the test suite. I will let you find and fix the issue.

Comment 28

[WangJun]

Attachment: patch

Comment 29

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

Comment on attachment 666957 [details] [diff] [review]
patch

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

Looks good. Let's test it: https://tbpl.mozilla.org/?tree=Try&rev=010f7c02c54e

Comment 30

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

Attachment: Companion fix

There is still an orange in the test suite, which does not seem related to WangJun's patch. Attaching a (self-reviewed) trivial fix.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47345d869884
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c01dd07cb2b

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47345d869884
https://hg.mozilla.org/mozilla-central/rev/0c01dd07cb2b