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

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

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

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

Comment 1

[Gaurav Rai [:gaurav0x]]

I want to work on this bug. I'm a newbie. Help me to start

Comment 2

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

If you are just getting started, I suggest starting with easier bugs, as this one requires a bit of experience.

Comment 3

[Gaurav Rai [:gaurav0x]]

I wanna do bit of coding. Give me a chance. I know javascript

Comment 4

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

Ok, then let's go. Have you already downloaded and built Firefox?
https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions

Comment 5

[Gaurav Rai [:gaurav0x]]

Yeah I have already done that. I have located the files in my source

Comment 6

[Gaurav Rai [:gaurav0x]]

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.

Comment 7

[Gaurav Rai [:gaurav0x]]

Attachment: Patch- Removing readTo() method and fixing read()

Comment 8

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

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.

Comment 9

[Gaurav Rai [:gaurav0x]]

Attachment: Removed readTo() and fixed read() ,tests and modified comments

I made the changes as asked by you. Inform me if anything is left.

Comment 10

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

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?

Comment 11

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

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?

Comment 12

[Gaurav Rai [:gaurav0x]]

Sure. But can you help me locate the files in mochi that have readTo tests?

Comment 13

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

That's main_test_osfile_async.js and worker_test_osfile_front.js.

Comment 14

[Gaurav Rai [:gaurav0x]]

Attachment: Removed readTo() and fixed mochi & xpcshell tests

Comment 15

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

As mentioned over IRC, I will probably be slower than usual to review this patch, as I will be traveling.

Comment 16

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

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`.

Comment 17

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

Well, tests look good.
Could you make the final change so that we can land it?

Comment 18

[Gaurav Rai [:gaurav0x]]

Could you specify exact lines to remove because replacing with let view = file.read() results in test fail?

Comment 19

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

What's the failure?

Comment 20

[Gaurav Rai [:gaurav0x]]

http://pastebin.mozilla.org/6804007

Comment 21

[Gaurav Rai [:gaurav0x]]

Any idea what this failure is about?

Comment 22

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

Ah, right. This wasy `let view = yield file.read()`.

Comment 23

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

Did this do the trick?

Comment 24

[Gaurav Rai [:gaurav0x]]

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

Comment 25

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

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.

Comment 26

[Gaurav Rai [:gaurav0x]]

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

Comment 27

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

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.

Comment 28

[Gaurav Rai [:gaurav0x]]

What should i do to make it work. ?

Comment 29

[Gaurav Rai [:gaurav0x]]

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 30

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

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;
}

Comment 31

[Gaurav Rai [:gaurav0x]]

Attachment: Removed readTo() and fixed read() ,fixed xpcshell and mochi tests and modified comments

Comment 32

[Gaurav Rai [:gaurav0x]]

Attachment: Removed readTo() and fixed read() ,fixed xpcshell and mochi tests and modified comments

Comment 33

[Gaurav Rai [:gaurav0x]]

I have also updated toolkit/components/osfile/modules/osfile_shared_front.jsm as specified

Comment 34

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

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

Comment 35

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

Let's try and land this.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/48099863baec

Comment 37

[Gaurav Rai [:gaurav0x]]

How to try and land it?

Comment 38

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

Ryan just did this in comment 36.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/48099863baec

Comment 40

[Gaurav Rai [:gaurav0x]]

okay david and thanks for helping me fix it :)

Comment 41

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

Congratulations, Gaurav :)
If you haven't done so yet, it's time to open an account on mozillians.org!

Comment 42

[Octoploid]

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

*************************

Comment 43

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

Oh, that's bad. We need to revert the change.

Comment 44

[Carsten Book [:Tomcat]]

(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

Comment 45

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

We need to wait until AdBlock Plus has removed its dependency towards `readTo`.

Comment 46

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Bug #1090958 is fixed now, so we can reland this patch if you want.

Comment 47

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

Let's reland.

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/80dc5596d158

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80dc5596d158

Comment 50

[Chris Peterson [:cpeterson]]

This change also broke Adblock Edge: bug 1103561.