Closed
Bug 997886
Opened 10 years ago
Closed 10 years ago
Test installing and updating apps with asm.js pre-compiling
Categories
(Core Graveyard :: DOM: Apps, defect)
Tracking
(firefox29 wontfix, firefox30 wontfix, firefox31 fixed, b2g-v1.4 affected, b2g-v2.0 fixed)
RESOLVED
FIXED
mozilla31
People
(Reporter: marco, Assigned: marco)
References
Details
Attachments
(1 file, 4 obsolete files)
15.04 KB,
patch
|
marco
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•10 years ago
|
||
In case you hadn't seen, bug 980447 added a "persistenceType" argument to nsIQuotaManager.clearStoragesForURI. This should allow you to write the following type of test which I think would be valuable: install app assert isAsmJSModuleLoadedFromCache quotaManager.clearStoragesForURI(uri, appid, false, "temporary"); assert isAsmJSModuleLoadedFromCache upgrade app assert isAsmJSModuleLoadedFromCache quotaManager.clearStoragesForURI(uri, appid, false, "temporary"); assert isAsmJSModuleLoadedFromCache quotaManager.clearStoragesForURI(uri, appid, false); assert !isAsmJSModuleLoadedFromCache
Assignee | ||
Comment 2•10 years ago
|
||
This test: 1) Installs several apps with wrong precompile fields and with an asm.js module 2) Asserts the module is not loaded from the cache 3) Installs an app, with an asm.js module 4) Asserts the module is loaded from the cache 5) Updates the app, with a different asm.js module 6) Asserts the module is loaded from the cache
Assignee: nobody → mar.castelluccio
Attachment #8408401 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8408604 -
Flags: review?(fabrice)
Assignee | ||
Updated•10 years ago
|
Summary: Test installing apps with asm.js pre-compiling → Test installing and updating apps with asm.js pre-compiling
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #1) > In case you hadn't seen, bug 980447 added a "persistenceType" argument to > nsIQuotaManager.clearStoragesForURI. This should allow you to write the > following type of test which I think would be valuable: > > install app > assert isAsmJSModuleLoadedFromCache > quotaManager.clearStoragesForURI(uri, appid, false, "temporary"); > assert isAsmJSModuleLoadedFromCache > upgrade app > assert isAsmJSModuleLoadedFromCache > quotaManager.clearStoragesForURI(uri, appid, false, "temporary"); > assert isAsmJSModuleLoadedFromCache > quotaManager.clearStoragesForURI(uri, appid, false); > assert !isAsmJSModuleLoadedFromCache Thank you, I'll add this additional test! (here or in a follow-up)
Comment 4•10 years ago
|
||
Comment on attachment 8408604 [details] [diff] [review] Patch Review of attachment 8408604 [details] [diff] [review]: ----------------------------------------------------------------- almost ready, thanks Marco! But can you add a lot more comments? Especially to explain the tricks used in asmjs_app.sjs ::: dom/apps/tests/asmjs/asmjs_app.sjs @@ +25,5 @@ > + var precompile = getState("precompile"); > + > + // Serve the application package corresponding to the requested app version. > + if ("getPackage" in query) { > + aResponse.setHeader("Content-Type", "application/java-archive", false); this is what we use elsewhere for jars, but that's not the correct mime type for zips. It looks like application/zip would be. ::: dom/apps/tests/test_packaged_app_asmjs.html @@ +34,5 @@ > +ScriptPreloader._enabled = true; > + > +SimpleTest.registerCleanupFunction(() => { > + WebappOSUtils = oldWebappOSUtils; > + ScriptPreloader._enabled = false; we probably don't want to do that on b2g. Let's do the save old value/restore dance... @@ +98,5 @@ > + } else if (message == "DONE") { > + ifr.removeEventListener('mozbrowsershowmodalprompt', onAlert, false); > + domParent.removeChild(ifr); > + aCallback(); > + } nit: trailing whitespace
Attachment #8408604 -
Flags: review?(fabrice) → feedback+
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8408604 -
Attachment is obsolete: true
Attachment #8409105 -
Flags: review?(fabrice)
Assignee | ||
Comment 6•10 years ago
|
||
I've also triggered a try run: https://tbpl.mozilla.org/?tree=Try&rev=7f1d0c0ede99
Comment 7•10 years ago
|
||
Comment on attachment 8409105 [details] [diff] [review] Patch Review of attachment 8409105 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +1389,5 @@ > aManifestURL); > return; > } > > + let manifest = new ManifestHelper(aJSON, app.manifestURL); While we fix these, do you mind changing https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1691 to be ManifestHelper(manifest, aData.manifestURL) ?
Attachment #8409105 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Carrying r+.
Attachment #8409105 -
Attachment is obsolete: true
Attachment #8409339 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d525f195556d
Flags: in-testsuite+
Keywords: checkin-needed
Comment 10•10 years ago
|
||
Backed out for the new test failing intermittently. https://hg.mozilla.org/integration/mozilla-inbound/rev/78b0aaa91320 https://tbpl.mozilla.org/php/getParsedLog.php?id=38194270&tree=Mozilla-Inbound
Flags: in-testsuite+
Assignee | ||
Comment 11•10 years ago
|
||
Is that the only failure? I've tried to reproduce it to have more logs, but it hasn't failed for several try runs (https://tbpl.mozilla.org/?tree=Try&rev=f7eb35454f33, https://tbpl.mozilla.org/?tree=Try&rev=ba7ea780ee4f, https://tbpl.mozilla.org/?tree=Try&rev=2662b8790b17, https://tbpl.mozilla.org/?tree=Try&rev=ea76b2e008a3)
Assignee | ||
Comment 12•10 years ago
|
||
Ryan, given that the failure is most probably the same as bug 915789, do you think we can land this anyway and file a bug to fix the intermittent failure (or re-use bug 915789 for this too)?
Flags: needinfo?(ryanvm)
Comment 13•10 years ago
|
||
Given the ongoing quality issues we're already having with the apps tests and the overall high intermittent failure rate we've got on trunk, I'm not inclined to knowingly add even more flakiness to the testsuite.
Flags: needinfo?(ryanvm)
Comment 14•10 years ago
|
||
Also please use the retrigger command in TBPL (blue cross, bottom of the UI) rather than making multiple try pushes :-)
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13) > Given the ongoing quality issues we're already having with the apps tests > and the overall high intermittent failure rate we've got on trunk, I'm not > inclined to knowingly add even more flakiness to the testsuite. Ok, then we need to fix bug 915879 (and probably 947565) first.
Depends on: 915879
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Marco Castelluccio [:marco] from comment #15) > (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13) > > Given the ongoing quality issues we're already having with the apps tests > > and the overall high intermittent failure rate we've got on trunk, I'm not > > inclined to knowingly add even more flakiness to the testsuite. > > Ok, then we need to fix bug 915879 (and probably 947565) first. Actually the test is only failing on Windows 8, so I guess we may disable it just on that platform if it's important to test this feature.
Assignee | ||
Comment 17•10 years ago
|
||
Rebased to inbound. Looks like with the patch from bug 915879, the test isn't intermittently failing anymore: https://tbpl.mozilla.org/?tree=Try&rev=89dab7f15f7c
Attachment #8409339 -
Attachment is obsolete: true
Attachment #8413167 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b50bcf913c9b
Flags: in-testsuite+
Keywords: checkin-needed
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b50bcf913c9b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 20•10 years ago
|
||
Is there a bug for adding a test that uses clearStoragesForURI as suggested in comment 1? I think that would be quite valuable for ensuring that the cached asm.js code is going into persistent storage; otherwise the user may experience random full-compilation startup pauses.
Comment 21•10 years ago
|
||
Uplifted because the uplift of bug 992150 was not trivial. I verified locally that tests are passing: https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/fb6d0b1672e5
Updated•10 years ago
|
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
status-firefox29:
--- → wontfix
status-firefox30:
--- → wontfix
status-firefox31:
--- → fixed
Comment 22•10 years ago
|
||
Backed out from 1.4 to check if this is the cause of the Moth failure at https://tbpl.mozilla.org/?tree=Mozilla-B2g30-v1.4&rev=fb6d0b1672e5 https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/50be03cea340
Comment 23•10 years ago
|
||
It was. I'm wondering if bug 915879 will help (note that I'm waiting on an approval request in that bug).
Assignee | ||
Comment 24•10 years ago
|
||
(In reply to Luke Wagner [:luke] (on partial PTO) from comment #20) > Is there a bug for adding a test that uses clearStoragesForURI as suggested > in comment 1? I think that would be quite valuable for ensuring that the > cached asm.js code is going into persistent storage; otherwise the user may > experience random full-compilation startup pauses. Filed bug 1005115.
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
•