Closed Bug 1075438 Opened 5 years ago Closed 5 years ago

[OS.File] Get rid of method `readTo`

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: Yoric, Assigned: gaurav0x, Mentored)

References

Details

(Keywords: addon-compat, dev-doc-needed, Whiteboard: [lang=js][good next bug])

Attachments

(1 file, 4 obsolete files)

The implementation of this method is extremely hairy, and will cause issues with the next iteration of the garbage-collector. Since nobody uses it anyway, we should remove it.

The objective of this bug is therefore to:
- remove OS.File.readTo;
- rewrite the small bits of code that depend on that function.

All the code lives here: 
http://dxr.mozilla.org/mozilla-central/search?q=readTo+path%3Atoolkit%2Fcomponents%2Fosfile+ext%3Ajsm&case=true
I want to work on this bug. I'm a newbie. Help me to start
If you are just getting started, I suggest starting with easier bugs, as this one requires a bit of experience.
I wanna do bit of coding. Give me a chance. I know javascript
Ok, then let's go. Have you already downloaded and built Firefox?
https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions
Yeah I have already done that. I have located the files in my source
I have tried to remove readTo() function but it results in test fail.I even removed the dependency on it 
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_async_front.jsm

The osfile_async_front.jsm readTo() function has no other function that depend upon it,still test get failed.
Attachment #8498918 - Flags: review?(dteller)
Assignee: nobody → gaurav_rai
Comment on attachment 8498918 [details] [diff] [review]
Patch- Removing readTo() method and fixing read()

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

This looks good.
Can you make the changes detailed below? Once this is done, we will send your patch to the Try server for unit testing.
Also, don't forget to add ",r=yoric" at the end of your commit message.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +617,5 @@
>        File.Info.fromMsg
>      );
>    },
>  
> +  

Nit: Please avoid a line of whitespace.
Hint: You may click on the Review link, they will be highlighted.

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +53,5 @@
>     * Read bytes from this file to a new buffer.
>     *
>     * @param {number=} bytes If unspecified, read all the remaining bytes from
>     * this file. If specified, read |bytes| bytes, or less if the file does notclone
>     * contain that many bytes.

Let's replace the documentation of `bytes` by
@param {number=} maybeBytes (deprecated, please use options.bytes)

@@ +57,5 @@
>     * contain that many bytes.
>     * @param {JSON} options
>     * @return {Uint8Array} An array containing the bytes read.
>     */
> +  read: function read(maybeBytes, options = {}) {

I realize that there is an error here. It was already here before you changed the code, but let's take the opportunity to fix it.

Callers of this function may use `write(bytes, options)`, `write(null, options)` or even `write(options)`. We do not handle the third case yet.

To handle this case, could you replace the first two lines of the function with:

if (typeof maybeBytes === "object") {
  // Caller has skipped `maybeBytes` and provided an options object.
  options = clone(maybeBytes);
  options.bytes = this.stat().size;
} else {
  options = clone(options || {});
  options.bytes = maybeBytes == null ? this.stat().size : maybeBytes;
}

@@ +62,3 @@
>      options = clone(options);
> +    options.bytes = maybeBytes == null ? this.stat().size : maybeBytes;
> +    let buffer = new Uint8Array(options.bytes);    

Nit: Please remove whitespace at the end of the line.

@@ +62,5 @@
>      options = clone(options);
> +    options.bytes = maybeBytes == null ? this.stat().size : maybeBytes;
> +    let buffer = new Uint8Array(options.bytes);    
> +    let {ptr, bytes} = SharedAll.normalizeToPointer(buffer, options.bytes);
> +    let size = 0;

Let's rename `size` into `pos`.

@@ +77,5 @@
>        return buffer;
>      } else {
>        return buffer.subarray(0, size);
>      }
> +    

Nit: Whitespace.
Attachment #8498918 - Flags: review?(dteller) → feedback+
I made the changes as asked by you. Inform me if anything is left.
Attachment #8498918 - Attachment is obsolete: true
Attachment #8499889 - Flags: review?(dteller)
Attachment #8499889 - Flags: feedback?
Comment on attachment 8499889 [details] [diff] [review]
Removed readTo() and fixed read() ,tests and modified comments

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

Looks good to me, with two tiny indentation fixes. You have my r+, but please upload an updated version once you have fixed these nits.
Let's run this through the Try Server: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=24700b799690

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +616,5 @@
>      return Scheduler.post("File_prototype_stat", [this._fdmsg], this).then(
>        File.Info.fromMsg
>      );
>    },
> +  

Nit: Can you remove the whitespaces here?

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +58,5 @@
>     */
> +  read: function read(maybeBytes, options = {}) {
> +    if (typeof maybeBytes === "object") {
> +    // Caller has skipped `maybeBytes` and provided an options object.
> +    options = clone(maybeBytes);

Nit: Can you indent the two blocks by 2 spaces?
Attachment #8499889 - Flags: review?(dteller)
Attachment #8499889 - Flags: review+
Attachment #8499889 - Flags: feedback?
Apparently, we forgot a few tests that use `readTo` – don't know why dxr didn't find them. Can you please get rid of the `readTo` tests in directory toolkit/components/osfile/tests/mochi?
Sure. But can you help me locate the files in mochi that have readTo tests?
That's main_test_osfile_async.js and worker_test_osfile_front.js.
No longer blocks: 1061288
Attachment #8499889 - Attachment is obsolete: true
Attachment #8503607 - Flags: review?(dteller)
As mentioned over IRC, I will probably be slower than usual to review this patch, as I will be traveling.
Comment on attachment 8503607 [details] [diff] [review]
Removed readTo() and fixed mochi & xpcshell tests

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

I believe that there is only one thing to fix.
Let's run it through Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=088a1913f2f2

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +229,5 @@
>      try {
>        let stat = yield file.stat();
>        test.info("Obtained file length");
>  
>        let view = new Uint8Array(stat.size);

That's not how `file.read` works.
You should rather replace this with
`let view = file.read()`

Same thing for `view2`.
Attachment #8503607 - Flags: review?(dteller) → review+
Well, tests look good.
Could you make the final change so that we can land it?
Could you specify exact lines to remove because replacing with let view = file.read() results in test fail?
What's the failure?
Any idea what this failure is about?
Flags: needinfo?(dteller)
Ah, right. This wasy `let view = yield file.read()`.
Flags: needinfo?(dteller)
Did this do the trick?
Flags: needinfo?(gaurav_rai)
No. It's not working. Do yield file.read() returns any value that can be assigned to view? I'm doubting it because it is written alone in that file without being assigned to anything
Flags: needinfo?(gaurav_rai)
Normally, `file.read()` returns a `Promise<Uint8Array>`, so `yield file.read()` should be a `Uint8Array`, which is what we need. To find out more about the contents of `yield file.read()`, you can use `dump`, e.g.

let view = yield file.read();
dump("view: " + view + "\n");
dump("stringified: " + JSON.stringify(view) + "\n");
dump("keys: " + Object.keys(view).join(", ") + "\n");

this will print some information on `view` on the system console.
After changing the file, code is http://pastebin.mozilla.org/6896954


The value of variables got after dump is http://pastebin.mozilla.org/6897070

The failed tests are http://pastebin.mozilla.org/6897088
So, apparently, the problem is that we now have `0` as position.
Apparently, `read()` doesn't change the position returned by `getPosition()`. That's weird.
What should i do to make it work. ?
The failed tests are http://pastebin.mozilla.org/6943456

The values of variables after dump is http://pastebin.mozilla.org/6943489

The modified code is http://pastebin.mozilla.org/6943490
Comment on attachment 8503607 [details] [diff] [review]
Removed readTo() and fixed mochi & xpcshell tests

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

Found the issue in your test.
Lines 244, 245. You should make the same change for `view2` as you did for `view`.

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +63,5 @@
> +      options.bytes = this.stat().size;
> +    } else {
> +      options = clone(options || {});
> +      options.bytes = maybeBytes == null ? this.stat().size : maybeBytes;
> +    }

Oh, it looks like we are ignoring `options.bytes` if it is already set. It's not the issue, but let's fix it.

if (typeof maybeBytes === "object") {
  // Caller has skipped `maybeBytes` and provided an `option` object as first arg.
  options = clone(maybeBytes);
  maybeBytes = null;
} else {
  options = clone(options || {});
}
if (!("bytes" in options)) {
  options.bytes = maybeBytes == null ? this.stat().size : maybeBytes;
}
Attachment #8503607 - Attachment is obsolete: true
Attachment #8511967 - Flags: review?(dteller)
Attachment #8511967 - Attachment is obsolete: true
Attachment #8511967 - Flags: review?(dteller)
Attachment #8511975 - Flags: review?(dteller)
I have also updated toolkit/components/osfile/modules/osfile_shared_front.jsm as specified
Comment on attachment 8511975 [details] [diff] [review]
Removed readTo() and fixed read() ,fixed xpcshell and mochi tests and modified comments

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

Looks good to me.
Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ac9923c18e1c
Attachment #8511975 - Flags: review?(dteller) → review+
Let's try and land this.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/48099863baec
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=js][good next bug] → [lang=js][good next bug][fixed-in-fx-team]
How to try and land it?
Ryan just did this in comment 36.
https://hg.mozilla.org/mozilla-central/rev/48099863baec
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good next bug][fixed-in-fx-team] → [lang=js][good next bug]
Target Milestone: --- → mozilla36
okay david and thanks for helping me fix it :)
Congratulations, Gaurav :)
If you haven't done so yet, it's time to open an account on mozillians.org!
Well, it breaks AbBlock Plus on my system. All my custom filters are not loaded anymore.
The log shows:

markus@x4 tmp % /usr/lib/firefox/firefox
*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: f.readTo is not a function
Full stack: exports.IO.readFromFile/<@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/markus/.mozilla/firefox/yjv8h6oo.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/bootstrap.js -> jar:file:///home/markus/.mozilla/firefox/yjv8h6oo.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/lib/io.js:179:28
TaskImpl_run@resource://gre/modules/Task.jsm:330:41
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7

*************************
*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: f.readTo is not a function
Full stack: exports.IO.readFromFile/<@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/markus/.mozilla/firefox/yjv8h6oo.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/bootstrap.js -> jar:file:///home/markus/.mozilla/firefox/yjv8h6oo.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/lib/io.js:179:28
TaskImpl_run@resource://gre/modules/Task.jsm:330:41
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7

*************************
*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: f.readTo is not a function
Full stack: exports.IO.readFromFile/<@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/markus/.mozilla/firefox/yjv8h6oo.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/bootstrap.js -> jar:file:///home/markus/.mozilla/firefox/yjv8h6oo.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/lib/io.js:179:28
TaskImpl_run@resource://gre/modules/Task.jsm:330:41
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7

*************************
*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: f.readTo is not a function
Full stack: exports.IO.readFromFile/<@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/markus/.mozilla/firefox/yjv8h6oo.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/bootstrap.js -> jar:file:///home/markus/.mozilla/firefox/yjv8h6oo.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/lib/io.js:179:28
TaskImpl_run@resource://gre/modules/Task.jsm:330:41
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7

*************************
*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: f.readTo is not a function
Full stack: exports.IO.readFromFile/<@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/markus/.mozilla/firefox/yjv8h6oo.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/bootstrap.js -> jar:file:///home/markus/.mozilla/firefox/yjv8h6oo.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/lib/io.js:179:28
TaskImpl_run@resource://gre/modules/Task.jsm:330:41
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7

*************************
*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: f.readTo is not a function
Full stack: exports.IO.readFromFile/<@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/markus/.mozilla/firefox/yjv8h6oo.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/bootstrap.js -> jar:file:///home/markus/.mozilla/firefox/yjv8h6oo.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/lib/io.js:179:28
TaskImpl_run@resource://gre/modules/Task.jsm:330:41
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7

*************************
Oh, that's bad. We need to revert the change.
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #43)
> Oh, that's bad. We need to revert the change.

backed out in remote:   https://hg.mozilla.org/mozilla-central/rev/366d06412304
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
We need to wait until AdBlock Plus has removed its dependency towards `readTo`.
Depends on: 1091664
Bug #1090958 is fixed now, so we can reland this patch if you want.
Flags: needinfo?(dteller)
Let's reland.
Flags: needinfo?(dteller)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/80dc5596d158
Keywords: checkin-needed
Whiteboard: [lang=js][good next bug] → [lang=js][good next bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/80dc5596d158
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good next bug][fixed-in-fx-team] → [lang=js][good next bug]
This change also broke Adblock Edge: bug 1103561.
You need to log in before you can comment on or make changes to this bug.