Closed Bug 839810 Opened 12 years ago Closed 12 years ago

Race condition installing apps on linux

Categories

(Core Graveyard :: DOM: Apps, defect)

x86
macOS
defect
Not set
normal

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
Attached file Log: Mac - Working (deleted) —
Attached file Log: Linux - Broken (deleted) —
Blocks: 826058
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
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...
Attached patch patch (obsolete) (deleted) — Splinter Review
Works for me when installing hosted+appcache apps from http://mozqa.com/webapi-permissions-tests/
Assignee: nobody → fabrice
Attachment #712797 - Flags: feedback?(dale)
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?
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.
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+
Attached patch patch v2 (deleted) — Splinter Review
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)
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+
Attachment #713101 - Flags: review?(ferjmoreno)
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+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: