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

RESOLVED FIXED in mozilla18

Status

()

Toolkit
OS.File
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: WangJun)

Tracking

unspecified
mozilla18
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 8 obsolete attachments)

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]
(Assignee)

Comment 2

5 years ago
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
(Assignee)

Comment 4

5 years ago
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
(Assignee)

Comment 6

5 years ago
Created attachment 665389 [details] [diff] [review]
patch

Is it the right one ?
Attachment #665389 - Flags: review?
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 8

5 years ago
Created attachment 665750 [details] [diff] [review]
simple patch

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

Comment 10

5 years ago
Created attachment 666143 [details] [diff] [review]
patch
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+
(Assignee)

Comment 12

5 years ago
Created attachment 666208 [details] [diff] [review]
the patch for bug
Attachment #666208 - Flags: review?(dteller)
(Assignee)

Comment 13

5 years ago
Created attachment 666209 [details] [diff] [review]
for the test
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
(Assignee)

Comment 17

5 years ago
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.
(Assignee)

Comment 19

5 years ago
Created attachment 666281 [details] [diff] [review]
patch for bug 793435
(Assignee)

Updated

5 years ago
Attachment #666281 - Flags: checkin?
(Assignee)

Updated

5 years ago
Attachment #666281 - Flags: checkin?
(Assignee)

Comment 20

5 years ago
Created attachment 666283 [details] [diff] [review]
patch for bug
Attachment #666283 - Flags: checkin?(dteller)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Updated

5 years ago
Attachment #666208 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #666209 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
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|.
(Assignee)

Comment 24

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #666283 - Attachment is obsolete: true
Attachment #666283 - Flags: review+
(Assignee)

Comment 25

5 years ago
Created attachment 666508 [details] [diff] [review]
Based on the original version
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.
(Assignee)

Updated

5 years ago
Attachment #666508 - Attachment is obsolete: true
Attachment #666508 - Flags: review?(dteller)
(Assignee)

Comment 28

5 years ago
Created attachment 666957 [details] [diff] [review]
patch
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+
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.
Attachment #667383 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/47345d869884
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c01dd07cb2b
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/47345d869884
https://hg.mozilla.org/mozilla-central/rev/0c01dd07cb2b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.