Last Comment Bug 793435 - [OS.File] fd.dispose is not a function
: [OS.File] fd.dispose is not a function
Status: RESOLVED FIXED
[mentor=Yoric][lang=js]
:
Product: Toolkit
Classification: Components
Component: OS.File (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla18
Assigned To: WangJun
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-22 12:27 PDT by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2012-10-03 18:56 PDT (History)
3 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (12.64 KB, patch)
2012-09-27 06:01 PDT, WangJun
no flags Details | Diff | Review
simple patch (1.62 KB, patch)
2012-09-27 20:45 PDT, WangJun
no flags Details | Diff | Review
patch (1.58 KB, patch)
2012-09-28 19:26 PDT, WangJun
dteller: feedback+
Details | Diff | Review
the patch for bug (1.56 KB, patch)
2012-09-29 08:12 PDT, WangJun
dteller: review+
Details | Diff | Review
for the test (1.26 KB, patch)
2012-09-29 08:13 PDT, WangJun
dteller: review+
Details | Diff | Review
patch for bug 793435 (2.82 KB, patch)
2012-09-29 21:56 PDT, WangJun
no flags Details | Diff | Review
patch for bug (3.09 KB, patch)
2012-09-29 22:06 PDT, WangJun
no flags Details | Diff | Review
Based on the original version (3.04 KB, patch)
2012-10-01 05:41 PDT, WangJun
no flags Details | Diff | Review
patch (3.06 KB, patch)
2012-10-02 06:01 PDT, WangJun
dteller: review+
Details | Diff | Review
Companion fix (2.24 KB, patch)
2012-10-03 01:57 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-22 12:27:39 PDT
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 Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-24 03:21:09 PDT
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 2012-09-26 03:29:11 PDT
Can I take this one, sir?
Only turn it into noop and run the test is enough?
Comment 3 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-26 03:42:31 PDT
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 2012-09-26 05:44:21 PDT
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 Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-26 06:21:27 PDT
From your object directory, after compilation:
  TEST_PATH=toolkit/components/osfile/tests/mochi make mochitest-chrome
Comment 6 WangJun 2012-09-27 06:01:25 PDT
Created attachment 665389 [details] [diff] [review]
patch

Is it the right one ?
Comment 7 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-27 06:13:15 PDT
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 2012-09-27 20:45:10 PDT
Created attachment 665750 [details] [diff] [review]
simple patch

Sir, is it like this?
Comment 9 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-27 21:34:09 PDT
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 2012-09-28 19:26:23 PDT
Created attachment 666143 [details] [diff] [review]
patch
Comment 11 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-28 19:49:32 PDT
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 2012-09-29 08:12:57 PDT
Created attachment 666208 [details] [diff] [review]
the patch for bug
Comment 13 WangJun 2012-09-29 08:13:58 PDT
Created attachment 666209 [details] [diff] [review]
for the test
Comment 14 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-29 08:18:53 PDT
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 Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-29 08:21:03 PDT
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 Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-29 08:21:14 PDT
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 2012-09-29 08:24:19 PDT
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 Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-29 20:14:58 PDT
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 2012-09-29 21:56:21 PDT
Created attachment 666281 [details] [diff] [review]
patch for bug 793435
Comment 20 WangJun 2012-09-29 22:06:22 PDT
Created attachment 666283 [details] [diff] [review]
patch for bug
Comment 21 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-29 23:00:31 PDT
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] 2012-09-30 11:14:10 PDT
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 Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-10-01 04:44:52 PDT
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 2012-10-01 04:55:35 PDT
(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 2012-10-01 05:41:12 PDT
Created attachment 666508 [details] [diff] [review]
Based on the original version
Comment 26 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-10-01 06:32:54 PDT
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 Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-10-02 03:05:25 PDT
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 2012-10-02 06:01:10 PDT
Created attachment 666957 [details] [diff] [review]
patch
Comment 29 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-10-02 06:06:14 PDT
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 Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-10-03 01:57:33 PDT
Created attachment 667383 [details] [diff] [review]
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.

Note You need to log in before you can comment on or make changes to this bug.