Closed Bug 999220 Opened 10 years ago Closed 10 years ago

confirmInstall calls aInstallSuccessCallback before manifest written to disk

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla32

People

(Reporter: myk, Assigned: marco)

Details

Attachments

(1 file, 1 obsolete file)

confirmInstall calls _writeManifestFile to write the manifest to disk, which delegates to _writeFile, which uses NetUtils.asyncCopy, which is supposed to be asynchronous.

But confirmInstall doesn't wait for _writeFile to resolve its promise before calling its aInstallSuccessCallback, so a caller can think the install succeeded before the manifest is written to disk.

If the caller then calls something that tries to retrieve the app record, like mozApps.mgmt.getAll, then it can get a record with a "null" manifest.

And Webapps.jsm and Webapps.js will both cache that "null" manifest, so subsequent attempts to retrieve the record will return the same "null" manifest for the duration of the session, even after the manifest file is successfully written to disk.

This problem was more noticeable back when we switched to using using OS.File in _writeFile; in fact, it caused bug 991397.  Now that we're back to using NetUtils.asyncCopy, I can't reproduce it anymore, perhaps because, as mfinkle notes in bug 991397, comment 6, NetUtils.asyncCopy isn't as asynchronous as it sounds.

But in theory this bug still exists, since _writeFile is intended to be asynchronous, so we should fix the problem.

To fix this, we should make _writeManifestFile return _writeFile's promise, then make confirmInstall wait until the promise is resolved before calling aInstallSuccessCallback.
Some improvements we may be able to do to this function:
- autoInstall and forceSuccessAck serve the same purpose, remove one of them
- Instead of using autoInstall (or forceSuccessAck) to force the call to onInstallSuccessAck, confirmInstall callers could register a message listener for "Webapps:Install:Return:OK" and send a "Webapps:Install:Return:Ack"
- Split aData into several parameters (confirmInstall: function(aApp, aIsPackaged, ...), to make it clear what are confirmInstall's parameters
- The "webapps-installed" notification could be removed and its users could add a message listener (like browser/modules/WebappManager.jsm)
- aInstallSuccessCallback could be removed by using promises
- OperatorApps.jsm could directly set jsonManifest.package_path instead of making confirmInstall do it

Obviously some of these are mutually exclusive and some may not be feasible.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8410978 - Flags: review?(myk)
Comment on attachment 8410978 [details] [diff] [review]
Patch

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

This looks good to me!  But I'm not authorized to review dom/apps/ code, so feedback+ and requesting review from Fabrice.

::: dom/apps/src/Webapps.jsm
@@ +2501,5 @@
>        };
>      }
>  
> +    // We notify about the successful installation via mgmt.oninstall and the
> +    // corresponging DOMRequest.onsuccess event as soon as the app is properly

Nit: corresponging -> corresponding
Attachment #8410978 - Flags: review?(myk)
Attachment #8410978 - Flags: review?(fabrice)
Attachment #8410978 - Flags: feedback+
Attachment #8410978 - Flags: review?(fabrice) → review+
Attached patch PatchSplinter Review
Fixed typo.
Assignee: myk → mar.castelluccio
Attachment #8410978 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8414397 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3a9f212f6fd9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.