Closed Bug 793435 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Yoric, Assigned: w1990ww1990w)

Details

(Whiteboard: [mentor=Yoric][lang=js])

Attachments

(2 files, 8 obsolete files)

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.
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|.
Whiteboard: [mentor=Yoric][lang=js]
Can I take this one, sir?
Only turn it into noop and run the test is enough?
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.
Assignee: nobody → w1990ww1990w
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.
From your object directory, after compilation:
  TEST_PATH=toolkit/components/osfile/tests/mochi make mochitest-chrome
Attached patch patch (obsolete) — Splinter Review
Is it the right one ?
Attachment #665389 - Flags: review?
Attachment #665389 - Flags: review? → review?(dteller)
Attachment #665389 - Attachment is patch: true
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
Attachment #665389 - Flags: review?(dteller)
Attached patch simple patch (obsolete) — Splinter Review
Sir, is it like this?
Attachment #665750 - Flags: review?(dteller)
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.
Attachment #665750 - Flags: review?(dteller)
Attachment #665389 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
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
Attachment #666143 - Flags: feedback+
Attached patch the patch for bug (obsolete) — Splinter Review
Attachment #666208 - Flags: review?(dteller)
Attached patch for the test (obsolete) — Splinter Review
Attachment #666209 - Flags: review?(dteller)
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 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.
Attachment #666209 - Flags: review?(dteller) → review+
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.
Attachment #666208 - Flags: review?(dteller) → review+
Attachment #665750 - Attachment is obsolete: true
Attachment #666143 - Attachment is obsolete: true
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.
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.
Attached patch patch for bug 793435 (obsolete) — Splinter Review
Attachment #666281 - Flags: checkin?
Attachment #666281 - Flags: checkin?
Attached patch patch for bug (obsolete) — Splinter Review
Attachment #666283 - Flags: checkin?(dteller)
Attachment #666281 - Attachment is obsolete: true
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).
Attachment #666283 - Flags: checkin?(dteller) → review+
Attachment #666208 - Attachment is obsolete: true
Attachment #666209 - Attachment is obsolete: true
Keywords: checkin-needed
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
Keywords: checkin-needed
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|.
(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.
Attachment #666283 - Attachment is obsolete: true
Attachment #666283 - Flags: review+
Attached patch Based on the original version (obsolete) — Splinter Review
Attachment #666508 - Flags: review?(dteller)
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.
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.
Attachment #666508 - Attachment is obsolete: true
Attachment #666508 - Flags: review?(dteller)
Attached patch patchSplinter Review
Attachment #666957 - Flags: review?(dteller)
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
Attachment #666957 - Flags: review?(dteller) → review+
Attached patch Companion fixSplinter Review
There is still an orange in the test suite, which does not seem related to WangJun's patch. Attaching a (self-reviewed) trivial fix.
Attachment #667383 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/47345d869884
https://hg.mozilla.org/mozilla-central/rev/0c01dd07cb2b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.