Closed
Bug 839810
Opened 12 years ago
Closed 12 years ago
Race condition installing apps on linux
Categories
(Core Graveyard :: DOM: Apps, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla21
People
(Reporter: daleharvey, Assigned: fabrice)
References
Details
Attachments
(3 files, 1 obsolete file)
When running https://bugzilla.mozilla.org/show_bug.cgi?id=826058 on try we get consistent failures on Linux (only)
The issue is in
var request = navigator.mozApps.install(gCachedManifestURL);
request.onerror = cbError;
request.onsuccess = continueTest;
yield;
var app = request.result;
ok(app, "App is non-null");
if (app.installState == "pending") {
ok(true, "App is pending. Waiting for progress");
app.onprogress = function() ok(true, "Got download progress");
app.ondownloadsuccess = continueTest;
app.ondownloaderror = cbError;
yield;
}
On mac the manifests are downloaded, we create an observer for the appcache download progress, execution is returned to content who listens for the ondownloadsuccess event which is delivered
On linux we download the appache contents before returning to content, however the pending status is still reported and the content waits for ondownloadsuccess events which never comes
attaching logs
Reporter | ||
Comment 1•12 years ago
|
||
Reporter | ||
Comment 2•12 years ago
|
||
Reporter | ||
Comment 3•12 years ago
|
||
So as far as I can tell
On linux the offline cache finishes downloading before we send the install.success event which seems strange at least in its consistency but something we have to account for since the install.success is sent async
https://github.com/mozilla/mozilla-central/blob/master/dom/apps/src/Webapps.jsm#L1858
The offlineCache event cant update the app object that doesnt exist and the app object data sent to install.success is stale. I am assuming we should ensure the
this.broadcastMessage("Webapps:Install:Return:OK", aData);
sends the most recent information
Assignee | ||
Comment 4•12 years ago
|
||
The issue is not that Webapps:Install:Return:OK returns stale data. The new app object that we create on the content side at this point registers itself asynchronously to receive app state updates, and it looks like the appcache tries to fire ondownloadsuccess before this registration message (https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#396) is processed on the parent side.
I'm not sure yet how to fix this...
Assignee | ||
Comment 5•12 years ago
|
||
Works for me when installing hosted+appcache apps from http://mozqa.com/webapi-permissions-tests/
Assignee: nobody → fabrice
Attachment #712797 -
Flags: feedback?(dale)
Comment 6•12 years ago
|
||
Hmm...that patch looks like it using a sleep of some sorts. I don't know how well that will fare when we stick these mochitests in CI. This might result in fixing a subset of instances to get the tests to pass, but there could be the potential for intermittent failures still.
Couldn't you do some sort of wait for condition instead?
Assignee | ||
Comment 7•12 years ago
|
||
For sure this is not the ideal patch, but there's no guarantee that the content will use the app object from install.onsuccess. So we have no straightforward way to solve that. I have an alternate patch in progress, but it's not working yet.
Reporter | ||
Comment 8•12 years ago
|
||
Comment on attachment 712797 [details] [diff] [review]
patch
Tests are passing with this patch, I had tried the same
Its a hack but I think in the interest of getting the tests in there it may be worth landing
(obviously need to remove the accidental whitespace change and would have a pointer to this bug in there)
Attachment #712797 -
Flags: feedback?(dale) → feedback+
Assignee | ||
Comment 9•12 years ago
|
||
Better approach, where we send back and acknowledgment from content to parent to make sure the webpage has a chance to add event handlers and that the app object registers its listeners before we download the appcache.
Attachment #712797 -
Attachment is obsolete: true
Attachment #713101 -
Flags: feedback?(dale)
Reporter | ||
Comment 10•12 years ago
|
||
Comment on attachment 713101 [details] [diff] [review]
patch v2
Approach definitely makes sense and its passing tests here, cheers fabrice
Attachment #713101 -
Flags: feedback?(dale) → feedback+
Assignee | ||
Updated•12 years ago
|
Attachment #713101 -
Flags: review?(ferjmoreno)
Comment 11•12 years ago
|
||
Comment on attachment 713101 [details] [diff] [review]
patch v2
Review of attachment 713101 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/apps/src/Webapps.jsm
@@ +1770,5 @@
> + // content side. This let the webpage the opportunity to set event handlers
> + // on the app before we start firing progress events.
> + queuedDownload: {},
> +
> + onInstallSuccessAck : function onInstallSuccessAck(aManifestURL) {
nit: extra space before ':'
Attachment #713101 -
Flags: review?(ferjmoreno) → review+
Assignee | ||
Comment 12•12 years ago
|
||
Comment 13•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•