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

RESOLVED FIXED in Firefox 28

Status

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

({dev-doc-needed})

unspecified
1.3 Sprint 6 - 12/6
All
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 unaffected)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment)

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.
Whiteboard: [MemShrink]
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.
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.
Attachment #8341547 - Flags: review?(dteller)
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.
Attachment #8341547 - Flags: review?(dteller) → review+
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.
blocking-b2g: --- → koi?
https://hg.mozilla.org/mozilla-central/rev/b843867654c3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 6 - 12/6
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?
Flags: needinfo?(fabrice)
(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.
Flags: needinfo?(fabrice)
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.
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.
blocking-b2g: koi? → koi+
Need a brief conversation with fabrice
blocking-b2g: koi+ → koi?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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?
yes, they should both be koi+. Let's use my blocking powers.
blocking-b2g: koi? → koi+
Whiteboard: [MemShrink] → [MemShrink][tarako]
status-b2g-v1.3T? and remove [tarako]
Whiteboard: [MemShrink][tarako] → [MemShrink]
Depends on: 1353731
You need to log in before you can comment on or make changes to this bug.