Closed Bug 989326 Opened 6 years ago Closed 5 years ago

Loader.jsm should neither flush nor perform main thread I/O

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: Yoric, Assigned: akshendra521994, Mentored)

Details

(Keywords: main-thread-io, Whiteboard: [Async][lang=js])

Attachments

(1 file, 3 obsolete files)

In Loader.jsm, method _writeFile flushes after every write and performs accidental main thread I/O. It would be more efficient to use OS.File.writeAtomic instead.
Just a note that _writeFile method is part of SRCDIR loader, which is only used by devtools developer who are hacking on devtools code and want auto reload of code.
It's still a blight on the code :)

More seriously, this appeared when I was searching for sources of accidental main thread I/O, so I filed the bug. The fix would be quite trivial.

If someone is interested in this bug, the code to fix is here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/Loader.jsm?from=Loader.jsm#177
Whiteboard: [Async][mentor=Yoric][lang=js]
Assigning to vedavidh as per in-person request at IITKGP MozSetup.
Assignee: nobody → vedavidhbudimuri
Status: NEW → ASSIGNED
Attached patch async.patch (obsolete) — Splinter Review
I think I fixed this, can you have a look?
Attachment #8399066 - Flags: review?(dteller)
Comment on attachment 8399066 [details] [diff] [review]
async.patch

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

Good start, thanks for the patch.
Could you make the changes below?

::: toolkit/devtools/Loader.jsm
@@ +175,5 @@
>  
> + _writeFile: function(filename, data) {
> +    let encoder = new TextEncoder();
> +    let array = encoder.encoder(data);
> +    let promise = OS.File.writeAtomic(filename,array);

Just |OS.File.writeAtomic(filename, data, { encoding: "utf-8"})| will do the trick.

@@ +176,5 @@
> + _writeFile: function(filename, data) {
> +    let encoder = new TextEncoder();
> +    let array = encoder.encoder(data);
> +    let promise = OS.File.writeAtomic(filename,array);
> +    promise.then(,function() {

That's a syntax error. You should pass |null| as first argument of |then|.

@@ +177,5 @@
> +    let encoder = new TextEncoder();
> +    let array = encoder.encoder(data);
> +    let promise = OS.File.writeAtomic(filename,array);
> +    promise.then(,function() {
> +      new Error("Couldn't write manifest: " + filename + "\n");

Don't just create the error, you should throw it.
Also, let's take the opportunity to make the message more precise:

promise.then(null, function(ex) {
 new Error("Couldn't write manifest: " + ex + "\n");
});

@@ +182,2 @@
>      });
> +    return promise;

You should return the result of the call to |then|, instead of just |promise|.
Attachment #8399066 - Flags: review?(dteller) → feedback+
Attached patch patch v2 (obsolete) — Splinter Review
i have made the changes that you have asked for. could you please  go through this new version and check it?
Attachment #8399066 - Attachment is obsolete: true
Attachment #8399078 - Flags: review?(dteller)
Comment on attachment 8399078 [details] [diff] [review]
patch v2

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

Looks good to me, thanks for the patch.
Don't forget to fold both patches in one.
Try Server results: https://tbpl.mozilla.org/?tree=Try&rev=4bf1a5dae897
Attachment #8399078 - Flags: review?(dteller) → review+
Comment on attachment 8399078 [details] [diff] [review]
patch v2

Looks like this is a patch on top of patch v1, so calling it "patch v2" is a bit misleading :) You should make sure to attach a patch that combines both for someone to check it in.
@Vedavidh, to fold the patches, do `hg qpop -a` (to pop all patches from the queue), then `hg qpush` (to put the first patch back on), and then `hg qfold nameofsecondpatch.patch`.

Edit the patch later ad make sure the commit details are consistent.
The tests look good. Vedavidh, once you have uploaded a folded patch, we'll be able to land this.
Flags: needinfo?(vedavidhbudimuri)
Comment on attachment 8399066 [details] [diff] [review]
async.patch

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

::: toolkit/devtools/Loader.jsm
@@ +172,5 @@
>      });
>      return deferred.promise;
>    },
>  
> + _writeFile: function(filename, data) {

The indentation here is now off, please fix it.
Vedavidh? Could you finish your patch?
Sorryy I couldn't make the change
Flags: needinfo?(vedavidhbudimuri)
Do you need assistance, vedavidh?
Flags: needinfo?(vedavidhbudimuri)
Hm, you can ask for help here, you know :)

What's the issue? Do you have the patches in your local repo?
actually i have formatted my system due to some problem
Flags: needinfo?(vedavidhbudimuri)
Hm. You can install Mercurial (sudo apt-get install mercurial on Ubuntu), and re-make the patch:

Download http://ftp.mozilla.org/pub/mozilla.org/firefox/bundles/mozilla-central.hg
mkdir mozilla-central
hg init mozilla-central
cd mozilla-central
hg unbundle /path/to/your/bundle.hg

Now download the two patches, and run hg qimport /path/to/patch

Now hg qpop -a, hg qpush nameoffirstpatch.patch, then hg qfold nameofsecondpatch.patch

Now make the edit to the file in comment 11, and `hg qref` to refresh the patch. Upload the new patch from mozilla-central/.hg/patches/nameofpatch.patch
vedavidh, the work is almost complete, so it would be great if you could finish it.
Flags: needinfo?(vedavidhbudimuri)
Mentor: dteller
Whiteboard: [Async][mentor=Yoric][lang=js] → [Async][lang=js]
Hi, Vedavidh - you're really close here, is there anything we can do to help you carry this over the line?
Looks like Vedavidh is not working on this bug anymore. Uniassigning.
Assignee: vedavidhbudimuri → nobody
Flags: needinfo?(vedavidhbudimuri)
Attached patch 989326_loader_jsmNoFlush.patch (obsolete) — Splinter Review
Attachment #8515478 - Flags: feedback?(dteller)
Comment on attachment 8515478 [details] [diff] [review]
989326_loader_jsmNoFlush.patch

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

Looks almost good.

::: toolkit/devtools/Loader.jsm
@@ +217,5 @@
>      });
>      return deferred.promise;
>    },
>  
> +  _writeFile: function(filename, data) {    

Nit: Could you remove the whitespace at the end of this line?

@@ +220,5 @@
>  
> +  _writeFile: function(filename, data) {    
> +    let promise = OS.File.writeAtomic(filename, data, {encoding: "utf-8"});
> +    return promise.then(null, (ex) => {
> +      new Error("Couldn't write manifest: " + ex + "\n");

Remove the curly braces, otherwise, the error handler will just produce `undefined`.
Attachment #8515478 - Flags: feedback?(dteller) → feedback+
Attachment #8515478 - Attachment is obsolete: true
Attachment #8515933 - Flags: review?(dteller)
Comment on attachment 8515933 [details] [diff] [review]
989326_loader_jsmNoFlush.patch

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

Looks good to me, thanks.
Attachment #8515933 - Flags: review?(dteller) → review+
Attachment #8399078 - Attachment is obsolete: true
Please assign that to me.
Assignee: nobody → akshendra521994
That looks good. Are you ready to land it?
Flags: needinfo?(akshendra521994)
How can I do that?
Flags: needinfo?(akshendra521994)
How to do that?
How to do that?
Just ask me :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3363feb38c14
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.