944859 - osfile_async_worker.js worker is never shut down in the parent process.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

We use OS.File in the parent but never use the api from bug 927560 to stop it. That make us waste 2.88MB in the parent. This happens both on 1.3 and 1.2.

I think that the code using OS.File is Webapps.jsm, and will provide a patch.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It's still not clear to me how to correctly and safely use the API from bug 927560.

Also we never landed that on 1.2.

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: auto-reset-worker.patch

Yoric, this patch adds "auto shutdown" support to OS.File. I first looked into explicitly calling resetWorker() in places where we use OS.File but we have too many scatered around to make that doable.

Comment 3

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

Comment on attachment 8341547 [details] [diff] [review]
auto-reset-worker.patch

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

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +159,5 @@
>      let options = methodArgs ? methodArgs[methodArgs.length - 1] : null;
>      let promise = worker.post.apply(worker, args);
>      return this.latestPromise = promise.then(
>        function onSuccess(data) {
> +        if (args[0] != "Meta_reset") {

This needs documentation.

@@ +191,5 @@
>          }
>          return data.ok;
>        },
>        function onError(error) {
> +        if (args[0] != "Meta_reset") {

Here, too.

Comment 4

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/b843867654c3

Comment 5

[[:fabrice] Fabrice Desré]

Asking for koi since that saves a few MB in the parent process. We would need to uplift Bug 927560 to 1.2 as well.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b843867654c3

Comment 7

[Preeti Raghunath(:Preeti)]

Fabrice,

927560 doesn't seem to be needed in 1.2.

Can you please confirm that the fix for 944859 will only be uplifting this patch?

Comment 8

[[:fabrice] Fabrice Desré]

(In reply to Preeti Raghunath(:Preeti) from comment #7)
> Fabrice,
> 
> 927560 doesn't seem to be needed in 1.2.
> 
> Can you please confirm that the fix for 944859 will only be uplifting this
> patch?

 Bug 927560 blocks this one so we need to uplift both.

Comment 9

[Jason Smith [:jsmith]]

Comments for 12/5 triage:

There's still confusion over the messaging that this bug states for it's blocking rationale vs. bug 927560's rationale for non-blocking. We need to discuss with Fabrice directly to understand this more.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We didn't take bug 927560 on 1.2 earlier because we took bug 927633 instead.  But now we need to take bug 927560 on 1.2 to take this on 1.2.  Not sure why this is hard to understand.

Comment 11

[Preeti Raghunath(:Preeti)]

Need a brief conversation with fabrice

Comment 12

[Jason Smith [:jsmith]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #10)
> We didn't take bug 927560 on 1.2 earlier because we took bug 927633 instead.
> But now we need to take bug 927560 on 1.2 to take this on 1.2.  Not sure why
> this is hard to understand.

Okay. Should bug 927560 be a koi? nom then as well?

Comment 13

[[:fabrice] Fabrice Desré]

yes, they should both be koi+. Let's use my blocking powers.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/41779abc39c7

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Bug 927560 didn't stick. Backed out.
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/6a05f0415dfb

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/d6f81ce77e7e

Comment 17

[Joe Cheng [:jcheng] (please needinfo)]

status-b2g-v1.3T? and remove [tarako]