Closed Bug 997886 Opened 6 years ago Closed 6 years ago

Test installing and updating apps with asm.js pre-compiling

Categories

(Core Graveyard :: DOM: Apps, defect)

x86_64
Linux
defect
Not set

Tracking

(firefox29 wontfix, firefox30 wontfix, firefox31 fixed, b2g-v1.4 affected, b2g-v2.0 fixed)

RESOLVED FIXED
mozilla31
Tracking Status
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 --- fixed
b2g-v1.4 --- affected
b2g-v2.0 --- fixed

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch WIP (obsolete) — Splinter Review
No description provided.
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
Attached patch Patch (obsolete) — Splinter Review
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)
Summary: Test installing apps with asm.js pre-compiling → Test installing and updating apps with asm.js pre-compiling
(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 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+
Attached patch Patch (obsolete) — Splinter Review
Attachment #8408604 - Attachment is obsolete: true
Attachment #8409105 - Flags: review?(fabrice)
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+
Attached patch Patch (obsolete) — Splinter Review
Carrying r+.
Attachment #8409105 - Attachment is obsolete: true
Attachment #8409339 - Flags: review+
Keywords: checkin-needed
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)
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)
Also please use the retrigger command in TBPL (blue cross, bottom of the UI) rather than making multiple try pushes :-)
(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
(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.
Attached patch PatchSplinter Review
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b50bcf913c9b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Blocks: 1002776
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.
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
It was. I'm wondering if bug 915879 will help (note that I'm waiting on an approval request in that bug).
Blocks: 1005115
(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.
Depends on: 1022665
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.