1268804 - Enable SecureContext for Storage API

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, defect, P2)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

The Storage API should be protected by the WebIDL annotation [SecureContext] from Bug 1267941.

Comment 1

[Andrew Overholt [:overholt]]

Do you plan to fold the annotation into the patch(es) in bug 1267941 (ie. should we keep this bug open as a distinct item?)?

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Andrew Overholt [:overholt] from comment #1)
> Do you plan to fold the annotation into the patch(es) in bug 1267941 (ie.
> should we keep this bug open as a distinct item?)?

I plan to fold the annotation into patch in bug 1267941 a few weeks ago, but later I found adding SecureContext API cannot be used. See Bug 1274315. Since some of storage types might need to use mochitest test suites to test with (ex:SpecialPowers), so I'm not sure this is correct things to add SecureContext annotation for now. This is why I did not close this bug.

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Ok, i will handle SecureContext in bug 1267941.

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Re-open this bug due to mochitests are not ready for SecureContext.

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Bug 1278370 will support https tests in .ini files for mochitests. If we need to add SecureContext annotation, we need to remove storage estimate mochitests.

See Bug 1278370 - support https tests in .ini files and harnesses.

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1268804.patch

Comment 8

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1268804.patch

Add worker test part.

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Bug 1269052 - Implement WorkerGlobalScope.isSecureContext is not finished. So we cannot access navigator.storage from worker.

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1268804.patch

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1268804.patch

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 - Use SecureContext for Storage API

Because navigator.storage cannot be accessed from worker if SecureContext enable. I tried to test worker version without SecureContext annotation, it works locally.

Comment 13

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 - Use SecureContext for Storage API

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8788084 [details] [diff] [review]
Bug 1268804 - Use SecureContext for Storage API

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

I'd like to revisit again after following issues are addressed, thanks!

::: dom/cache/test/mochitest/test_cache_orphaned_body.html
@@ -65,5 @@
> -    let worker = new Worker(url);
> -    worker.onmessage = function (e) {
> -      resolve(e.data, 0);
> -    };
> -  });

Can we move this to the wpt, if wpt can test it?
Otherwise, we need a follow-up bug for tracking this.

::: dom/indexedDB/test/mochitest.ini
@@ -95,5 @@
>    unit/test_setVersion_abort.js
>    unit/test_setVersion_events.js
>    unit/test_setVersion_exclusion.js
>    unit/test_setVersion_throw.js
> -  unit/test_storage_manager_estimate.js

We don't have to remove it.

@@ -356,5 @@
>  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116
>  [test_setVersion_throw.html]
>  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116
> -[test_storage_manager_estimate.html]
> -skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116

but modify the skip-if as:
skip-if = true # Bug XXXXXXX
for tracking.

::: testing/web-platform/meta/storage/estimate-worker.https.html.ini
@@ +6,5 @@
> +  [estimate() resolves to dictionary with members]
> +    expected: FAIL
> +
> +  [estimate() shows usage increase after large value is stored]
> +    expected: FAIL

We should have a follow-up bug for the test coverage in worker if not testable for now instead of having a non-testable test case uplifted into wpt. :)

::: testing/web-platform/tests/storage/estimate.https.html
@@ +3,5 @@
> +<script src="/resources/testharness.js"></script>
> +<script src="/resources/testharnessreport.js"></script>
> +<script>
> +const objectStoreName = "storagesManager";
> +const arraySize = 1e6;

move these const into the last promise_test().

@@ +21,5 @@
> +    assert_equals(typeof result.quota, 'number');
> +  });
> +}, 'estimate() resolves to dictionary with members');
> +
> +function deleteDB(name) {

ditto

@@ +29,5 @@
> +    del.onsuccess = function() { resolve(); };
> +  });
> +}
> +
> +function openDB(name, upgrade) {

ditto

@@ +41,5 @@
> +
> +promise_test(function(t) {
> +  const dbname = this.window ? window.location.pathname :
> +  	   "estimate.https.html";
> +  let db, before, usageAfterCreate, usageAfterPut;

usageAfterDelete shall also be tested here. :)

Comment 15

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #14)
> Comment on attachment 8788084 [details] [diff] [review]
> Bug 1268804 - Use SecureContext for Storage API
> 
> Review of attachment 8788084 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to revisit again after following issues are addressed, thanks!
> 
> ::: dom/cache/test/mochitest/test_cache_orphaned_body.html
> @@ -65,5 @@
> > -    let worker = new Worker(url);
> > -    worker.onmessage = function (e) {
> > -      resolve(e.data, 0);
> > -    };
> > -  });
> 
> Can we move this to the wpt, if wpt can test it?
> Otherwise, we need a follow-up bug for tracking this.
I will try to add tests in wpt.

> 
> ::: dom/indexedDB/test/mochitest.ini
> @@ -95,5 @@
> >    unit/test_setVersion_abort.js
> >    unit/test_setVersion_events.js
> >    unit/test_setVersion_exclusion.js
> >    unit/test_setVersion_throw.js
> > -  unit/test_storage_manager_estimate.js
> 
> We don't have to remove it.
If we can have wpt tests (the same test coverage) to replace it, I can we can remove it.
 

> 
> @@ -356,5 @@
> >  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116
> >  [test_setVersion_throw.html]
> >  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116
> > -[test_storage_manager_estimate.html]
> > -skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116
> 
> but modify the skip-if as:
> skip-if = true # Bug XXXXXXX
> for tracking.
> 
> ::: testing/web-platform/meta/storage/estimate-worker.https.html.ini
> @@ +6,5 @@
> > +  [estimate() resolves to dictionary with members]
> > +    expected: FAIL
> > +
> > +  [estimate() shows usage increase after large value is stored]
> > +    expected: FAIL
> 
> We should have a follow-up bug for the test coverage in worker if not
> testable for now instead of having a non-testable test case uplifted into
> wpt. :)
Ok, will do.
> 
> ::: testing/web-platform/tests/storage/estimate.https.html
> @@ +3,5 @@
> > +<script src="/resources/testharness.js"></script>
> > +<script src="/resources/testharnessreport.js"></script>
> > +<script>
> > +const objectStoreName = "storagesManager";
> > +const arraySize = 1e6;
> 
> move these const into the last promise_test().
> 
> @@ +21,5 @@
> > +    assert_equals(typeof result.quota, 'number');
> > +  });
> > +}, 'estimate() resolves to dictionary with members');
> > +
> > +function deleteDB(name) {
> 
> ditto
> 
> @@ +29,5 @@
> > +    del.onsuccess = function() { resolve(); };
> > +  });
> > +}
> > +
> > +function openDB(name, upgrade) {
> 
> ditto
> 
> @@ +41,5 @@
> > +
> > +promise_test(function(t) {
> > +  const dbname = this.window ? window.location.pathname :
> > +  	   "estimate.https.html";
> > +  let db, before, usageAfterCreate, usageAfterPut;
> 
> usageAfterDelete shall also be tested here. :)
Oh. I forgot to add it.

Comment 16

[Andrew Overholt [:overholt]]

Not having isSecureContext available in Workers (bug 1269052) is a potential blocker here. I suggested to Shawn that he email dev-platform to gauge people's thoughts on initially releasing the Storage API without Worker support.

Comment 17

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Andrew Overholt [:overholt] from comment #16)
> Not having isSecureContext available in Workers (bug 1269052) is a potential
> blocker here. I suggested to Shawn that he email dev-platform to gauge
> people's thoughts on initially releasing the Storage API without Worker
> support.

I've sent the email to dev-platform. Thanks for the suggestion :)

Comment 18

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Bug 1304966 - Disable Storage estimate() function due to lack of support SecureContext on workers

Comment 19

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

bug 1269052 landed, so i will enable SecureContext.

Comment 20

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #14)
> Comment on attachment 8788084 [details] [diff] [review]
> Bug 1268804 - Use SecureContext for Storage API
> 
> Review of attachment 8788084 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to revisit again after following issues are addressed, thanks! 
> ::: dom/indexedDB/test/mochitest.ini
> @@ -95,5 @@
> >    unit/test_setVersion_abort.js
> >    unit/test_setVersion_events.js
> >    unit/test_setVersion_exclusion.js
> >    unit/test_setVersion_throw.js
> > -  unit/test_storage_manager_estimate.js
> 
> We don't have to remove it.
Hi Bevis,
Bug 1286312 is still not ready, so mochitests still can't run via https channel.
My plan is to move StorageManager estimate mochitests test cases to wpt.

Comment 21

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1268804.patch

Comment 22

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1268804.patch

Comment 23

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8802032 [details] [diff] [review]
bug-1268804.patch

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

::: testing/web-platform/tests/storage/storage-estimate-cache.js
@@ +49,5 @@
> +  }).then(function() {
> +     navigator.storage.estimate()
> +    .then(storageEstimation =>{
> +      usageAfterDelete = storageEstimation.usage;
> +      dump("Storage cache: usage usageAfterDelete " + usageAfterDelete + '\n');

Hi Jan,
I'm trying to write cache test case for estimate() in wpt test cases, so convert estimate test in test_cache_orphaned_body.html.
Since mochitest doesn't support SecureContext.
However, I found even after calling |caches.delete()| and wait for two async steps like what mochitest cases did (#1).
estimate usage usageAfterDelete is not less than usageAfterAdd, even usageAfterDelete is large than usageAfterAdd.
Somehow MaybeUpdateSize() still be executed after cache.delete(). So it can't guarantee what we got estimate usage is correct after deletion.
But since wpt doesn't support SpecialPower, so I can't call |resetStorage()|. Does that mean we have limitation to test estimate() after deletion?
Shall I still include deletion test case here?

#1 http://searchfox.org/mozilla-central/source/dom/cache/test/mochitest/test_cache_shrink.html#112
#2 http://searchfox.org/mozilla-central/source/dom/cache/test/mochitest/test_cache_shrink.html#49

Comment 24

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1268804.patch

Pref-off StorageManager in release build and disable wpt test cases for release build.

Comment 25

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1268804.patch

Comment 26

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I probably should move pref-off code to Bug 1304966.

Comment 27

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8803256 [details] [diff] [review]
bug-1268804.patch

I found i made some mistakes on pref-off things in this patch.

Comment 28

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 - Enable SecureContext for Storage API

Comment 29

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I moved pref-off code to Bug 1304966. So this bug should only enable SecureContext and convert mochitest to wpt test cases.

Comment 30

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I still don't have any good solution to solve the problem addressed on Comment 23, I'm wondering if I can open an another follow-up bug to add tests for Cache.

Comment 31

[Jan Varga [:janv]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #30)
> I still don't have any good solution to solve the problem addressed on
> Comment 23, I'm wondering if I can open an another follow-up bug to add
> tests for Cache.

We talked about this online. Shawn now knows how to proceed.

Comment 32

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 - Enable SecureContext for Storage API

Comment 33

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 - Enable SecureContext for Storage API

Comment 34

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 - Enable SecureContext for Storage API

Comment 35

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

try:
https://hg.mozilla.org/try/pushloghtml?changeset=649b010aed3eb72822af16f64aa8f82d408881db

Comment 36

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=649b010aed3eb72822af16f64aa8f82d408881db

Comment 37

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Oh! No! I forgot test_interfaces.html, test_navigator.html, test_worker_interfaces.html also checks StorageManager. Now we probably need to figure out how to handle this situation since mochitest still can't be executed via https :( Even I convert Storage API test cases to wpt test cases.

* TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_interfaces.html | StorageManager should be defined on the global scope
* TEST-UNEXPECTED-FAIL | dom/workers/test/test_navigator.html | Worker had an error: uncaught exception: Navigator has no 'storage' property!
* TEST-UNEXPECTED-FAIL | dom/workers/test/test_worker_interfaces.html | false: StorageManager should be defined on the global scope

Comment 38

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hi baku,
Because StorageManager restricted to SecureContext (https://storage.spec.whatwg.org/#api), and there are several test cases checking interfaces correctly been exposed. However, these test cases (#1, #2, #3) loaded via http instead of https, so test cases can't access theses interfaces restricted to SecureContext. So I guess there is no other way to work around it, unless mochitest can run test cases via https. Current situation is mochitest still can't deal with this (Bug 1286312), but they are working on solution to load the specific test cases in https by adding attributes in mochitest.ini.

I'm wondering loading these interfaces test cases via https is the only correct solution? Do you have any other concern?

#1 dom/tests/mochitest/general/test_interfaces.html
#2 dom/workers/test/test_navigator.html
#3 dom/workers/test/test_worker_interfaces.html

Comment 39

[Andrea Marchesini [:baku]]

Moving the NI to jwatt.

Comment 40

[Jonathan Watt [:jwatt]]

I think the only solution is to fix bug 1286312 or else to rewrite the tests as web-platform-tests which supports creating tests that are secure contexts. We have some tests that you can crib from at:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/secure-contexts/

Comment 41

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Jonathan Watt [:jwatt] from comment #40)
> I think the only solution is to fix bug 1286312 or else to rewrite the tests
> as web-platform-tests which supports creating tests that are secure
> contexts. We have some tests that you can crib from at:
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/
> secure-contexts/

I see. Thank you.

Comment 42

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: add_https_mochitest_option (experimental only)

I enforced test cases run via https using the experimental patch from Bug 1286312, the following test cases can pass.

#1 dom/tests/mochitest/general/test_interfaces.html
#2 dom/workers/test/test_navigator.html
#3 dom/workers/test/test_worker_interfaces.html

Comment 43

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Maybe we can do a trick to set security.mixed_content.block_active_content to false?
Then we probably can ignore the restriction.

Comment 44

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #43)
> Maybe we can do a trick to set security.mixed_content.block_active_content
> to false?
> Then we probably can ignore the restriction.

This probably won't fix the problem.

Comment 45

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hi bz,
I'm a bit not sure you set dependency on bug 1315029. Do we have to resolve anything concrete or just discussion on bug 1315029?

Comment 46

[Boris Zbarsky [:bzbarsky]]

Is this API exposed in webextensions now?  Is it still exposed after this change?  If not, will that break any extensions?  Is it exposed in webextensions in Chrome?

Mostly we need to figure out whether we need to resolve bug 1315029 before we ship this in release...  It's hard right now to even say whether bug 1315029 is an issue, because we don't have any [SecureContext] APIs that we could use to check.

Comment 47

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #46)
> Is this API exposed in webextensions now?  Is it still exposed after this
> change?  If not, will that break any extensions?  Is it exposed in
> webextensions in Chrome?
> 
> Mostly we need to figure out whether we need to resolve bug 1315029 before
> we ship this in release...  It's hard right now to even say whether bug
> 1315029 is an issue, because we don't have any [SecureContext] APIs that we
> could use to check.

https://w3c.github.io/webappsec-secure-contexts/#packaged-applications
"A user agent that support packaged applications MAY consider as "secure" specific URL schemes whose contents are authenticated by the user agent. For example, FirefoxOS application resources are referred to by a URL whose scheme component is app:. Likewise, Chrome’s extensions and apps live on chrome-extension: schemes. These could reasonably be considered trusted origins."

https://www.w3.org/TR/secure-contexts/#is-origin-trustworthy
"On the other hand, the user agent MAY choose to extend this trust to other, vendor-specific URL schemes like app: or chrome-extension: which it can determine a priori to be trusted (see §7.1 Packaged Applications for detail)."

And I saw chrome add chrome-extension to the white-list
https://cs.chromium.org/chromium/src/chrome/common/secure_origin_whitelist.cc?sq=package:chromium&dr=CSs&rcl=1478708199&l=32

void GetSchemesBypassingSecureContextCheckWhitelist(
    std::set<std::string>* schemes) {
  schemes->insert(extensions::kExtensionScheme);
}

Comment 48

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

http://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#657
I guess we trust moz-extensions here.

Comment 49

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 - Enable SecureContext for Storage API

Comment 50

[Boris Zbarsky [:bzbarsky]]

OK, so we should make sure this API is exposed in webextensions.  Is it, with this patch?

Comment 51

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #50)
> OK, so we should make sure this API is exposed in webextensions.  Is it,
> with this patch?

Oh, I didn't consider webextension in the beginning, thanks for pointing out. I will figure out there is anything I missed.

Comment 52

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Bug 1308951 landed. Based on Bug 1274315 Comment 6.

SpecialPowers.pushPrefEnv({"set": [["dom.securecontext.whitelist",
"mochi.test"]]},
function() {
// do things that require SecureContext
});

Comment 53

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #52)
> Bug 1308951 landed. Based on Bug 1274315 Comment 6.
> 
> SpecialPowers.pushPrefEnv({"set": [["dom.securecontext.whitelist",
> "mochi.test"]]},
> function() {
> // do things that require SecureContext
> });

It seems nsContentSecurityManager checks that pref before loading mochitest test case.
So adding dom.securecontext.whitelist in test cases doesn't work.

Comment 54

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1286312 - Add mochitest option to run tests using https

Comment 55

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1286312 - cleanup with latest coding style

Comment 56

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 1: Enable SecureContext for Storage API

Comment 57

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 2: Add test cases for SecureContext

Comment 58

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://hg.mozilla.org/try/rev/68f2c91ab90abf00eb5437db3712ce5d24cb672f

Comment 59

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68f2c91ab90abf00eb5437db3712ce5d24cb672f

Comment 60

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b94a7305312b792a1b23bc7f3b2ef2052303a461

Comment 61

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #60)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b94a7305312b792a1b23bc7f3b2ef2052303a461

Let's see we had fixed all the problems we discussed for test_interfaces for window/worker!

Comment 62

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bd20cec7c8fa8f732b5acba2d327bf009f01ff7

Comment 63

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Opps. The patch (Bug 1286312 - Add mochitest option to run tests), it doesn't seem to work in try-server. But locally i executed the test case with mochitest.ini change, it can work properly.

Comment 64

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #63)
> Opps. The patch (Bug 1286312 - Add mochitest option to run tests), it
> doesn't seem to work in try-server. But locally i executed the test case
> with mochitest.ini change, it can work properly.

Discussed with bug owner of bug 1286312, that patch in bug 1286312 will be fixed in the next revision. So I will wait for that bug. Meanwhile, :jcj confirmed that setting pref dom.securecontext.whitelist in mochitest test case cannot work because nsContentSecurityManager checks pref earlier than loading mochitest.
https://bugzilla.mozilla.org/show_bug.cgi?id=1274315#c9

Comment 65

[Boris Zbarsky [:bzbarsky]]

> because nsContentSecurityManager checks pref earlier than loading mochitest

Why is that pref not live?  That seems wrong.

Comment 66

[Boris Zbarsky [:bzbarsky]]

Er, actually it is live.  Then I guess your problem is that the thing you run the setCharPref call in is already created so can't become a secure context, but then neither can any of the things it loads?

Comment 67

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

bug 1286312 just landed. So I will revise my patch again.

Comment 68

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c43969f61de30a75f0e6c4b015a7dd73105025cf&selectedJob=71146784

Hi :jcj
I mochitest scheme equals to https for dom/tests/mochitest/general/test_interfaces.html, and I found these error. Do you have any idea?

The following tests failed:
1475 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface ScopedCredentialInfo to all webpages as a property on the window (XBL: false)? Do not make a change to this file without a  review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals) 
runTest@https://example.com/tests/dom/tests/mochitest/general/test_interfaces.html:1376:5
@https://example.com/tests/dom/tests/mochitest/general/test_interfaces.html:1398:1
1476 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface WebAuthnAttestation to all webpages as a property on the window (XBL: false)? Do not make a change to this file without a  review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals) 
runTest@https://example.com/tests/dom/tests/mochitest/general/test_interfaces.html:1376:5
@https://example.com/tests/dom/tests/mochitest/general/test_interfaces.html:1398:1
1477 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface WebAuthentication to all webpages as a property on the window (XBL: false)? Do not make a change to this file without a  review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals) 
runTest@https://example.com/tests/dom/tests/mochitest/general/test_interfaces.html:1376:5
@https://example.com/tests/dom/tests/mochitest/general/test_interfaces.html:1398:1
1478 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface ScopedCredential to all webpages as a property on the window (XBL: false)? Do not make a change to this file without a  review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals) 
runTest@https://example.com/tests/dom/tests/mochitest/general/test_interfaces.html:1376:5
@https://example.com/tests/dom/tests/mochitest/general/test_interfaces.html:1398:1
1479 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface WebAuthnAssertion to all webpages as a property on the window (XBL: false)? Do not make a change to this file without a  review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals) 
runTest@https://example.com/tests/dom/tests/mochitest/general/test_interfaces.html:1376:5
@https://example.com/tests/dom/tests/mochitest/general/test_interfaces.html:1398:1
1480 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface ScopedCredentialInfo to all webpages as a property on the window (XBL: true)? Do not make a change to this file without a  review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals) - expected PASS
1481 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface WebAuthnAttestation to all webpages as a property on the window (XBL: true)? Do not make a change to this file without a  review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals) - expected PASS
1482 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface WebAuthentication to all webpages as a property on the window (XBL: true)? Do not make a change to this file without a  review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals) - expected PASS
1483 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface ScopedCredential to all webpages as a property on the window (XBL: true)? Do not make a change to this file without a  review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals) - expected PASS
1484 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface WebAuthnAssertion to all webpages as a property on the window (XBL: true)? Do not make a change to this file without a  review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals) - expected PASS

Comment 69

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #68)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c43969f61de30a75f0e6c4b015a7dd73105025cf&selectedJob=7
> 1146784
> 
> Hi :jcj
> I mochitest scheme equals to https for
> dom/tests/mochitest/general/test_interfaces.html, and I found these error.
typo: "Setting" mochitest scheme equals to https.

Comment 70

[Boris Zbarsky [:bzbarsky]]

> are you sure you want to expose the new interface WebAuthnAttestation 

That's because in bug 1309284 we added this interface as [SecureContext] but did NOT add it to test_interfaces.  And test_interfaces didn't notice, because it was running as http://.  That's horrible.  :(

We should add an annotation to test_interfaces that indicates which channels this API is expected to be exposed on.  I very much doubt that in 53 the answer is "all channels", but that's something to check with the authors/reviewers from bug 1309284.

Same thing for all the other failures; all those interfaces are part of the web authentication API.

Comment 71

[J.C. Jones [:jcj] (he/him)]

Ugh, will get a patch together as soon as my next hours' meetings are done. Sorry about this :shawnjohnjr. Opened Bug 1333084 for it.

Comment 72

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yeah, sorry, I read the prefs as cascading from the navigator object, which was incorrect. Looks like this is being taken care of on bug 1333084 now.

Comment 73

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 2: Add test cases for SecureContext

Rebase to the latest patch.

Comment 74

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec79f6b37b4ef1f7b26e313fd4e1593c3ece9689

Comment 75

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

[2017-01-25 09:53:44] <rpl> baku: yes, it is
[2017-01-25 09:53:57] <baku> shawnjohnjr: so yes, any DOM API is exposed.
[2017-01-25 09:54:13] <baku> shawnjohnjr: but writing a test is better.
[2017-01-25 09:54:42] <shawnjohnjr> baku: got it
[2017-01-25 09:58:05] <rpl> baku: in the background page some APIs have (or had in the past) some issues, but if it doesn't work, it is usually a bug (most of the time related to something missing on the windowless browsers that contain the hidden extension pages)
[2017-01-25 09:59:51] ⇐ Standard8 quit ([e-mail]): Ping timeout: 121 seconds
[2017-01-25 10:04:53] <shawnjohnjr> rpl: if we want to test one DOM API can correctly be exposed to webextensions, which folders I should put test cases into?
[2017-01-25 10:05:19] <shawnjohnjr> rpl: "browser/components/extensions/test/browser"?
[2017-01-25 10:06:01] ⇐ joncat quit ([e-mail]): Client exited
[2017-01-25 10:08:27] <rpl> shawnjohnjr: if the DOM API doesn't need any browser UI (or feature) to be tested then toolkit/components/extensions/test
[2017-01-25 10:09:48] <shawnjohnjr> rpl: cool, thanks
[2017-01-25 10:11:22] <rpl> shawnjohnjr: when it is possible we are creating them as xpcshell test, or as mochitest (plain or chrome), if the test needs stuff that is not available in an xpcshell
[2017-01-25 10:11:29] * noit is now known as noit-away
[2017-01-25 10:15:10] → joncat joined ([e-mail])
[2017-01-25 10:16:28] <shawnjohnjr> rpl: i see, so it's better to test that both background/content scripts can access that DOM API.
[2017-01-25 10:19:51] <rpl> shawnjohnjr: in general the content scripts access the DOM APIs by using the content window that they attach (and in this case most of the time the issues are related to the fact that the content script and the "attached" content window are running in different compartment)
[2017-01-25 10:23:32] <rpl> shawnjohnjr: to test that the DOM APIs are working in a WebExtension page, testing the them in the background page is often enough, but if it doesn't work, it is worth a try to test the issue in a extension tab (an extension url loaded in a tab), to check if the API works in a "regular environment"

Comment 76

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #74)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ec79f6b37b4ef1f7b26e313fd4e1593c3ece9689

Looks good now.

Comment 77

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 3: Test storage api in webextension

Comment 78

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8831132 [details] [diff] [review]
Bug 1268804 Part 3: Test storage api in webextension

Hi bz,
Can you give any feedback if this patch is in the right direction? I followed your Comment 50 and add webextension test. Thank you.

Comment 79

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8831132 [details] [diff] [review]
Bug 1268804 Part 3: Test storage api in webextension

I don't really know enough about webextensions to tell what this test is doing in practice, sorry.

The key part of such a test would be to:

1)  Load a page that is NOT secure-context.  Ideally the test would check this
    (e.g. by making sure it doesn't see the storage API).
2)  Verify that a webextension content-script running against that
    non-secure-context page does see the storage API.

Comment 80

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 3: Test storage api in webextension

Comment 81

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8833267 [details] [diff] [review]
Bug 1268804 Part 3: Test storage api in webextension

Hi rpl,
Do you mind reviewing the test case I added in webextensions for testing Storage API[1] in SecureContext and non-SecureContext? I'm trying to test Storage API can be accessed in background script and Storage API CANNOT be accessed in non-secure-context content script. I'm a little bit not sure my conclusion is correct here, since my observation is different from item 2 in Comment 79.

WebExtension content-script running against that non-secure-context page does not see the storage API.

[1] Storage API: https://storage.spec.whatwg.org/#idl-index

Comment 82

[Jan Varga [:janv]]

Comment on attachment 8830181 [details] [diff] [review]
Bug 1268804 Part 2: Add test cases for SecureContext

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

r=me, but I'd like to see updated patch before landing

::: dom/workers/test/mochitest.ini
@@ +212,5 @@
>  skip-if = toolkit == 'android' #bug 982828
>  [test_webSocket_sharedWorker.html]
>  skip-if = toolkit == 'android' #bug 982828
>  [test_worker_interfaces.html]
> +scheme=https

What about test_interfaces.html ?

Also, shouldn't we now check what's exposed for http and for https separately ?
I recommend to ask one of the DOM peers.

::: testing/web-platform/tests/storage/storage-estimate-indexeddb.js
@@ +17,5 @@
> +    assert_equals(typeof result.quota, 'number');
> +  });
> +}, 'estimate() resolves to dictionary with members');
> +
> +function deleteDB(name) {

Nit: it would be nicer to move this function the the promise test, before |return deleteDB(dbname)|

@@ +19,5 @@
> +}, 'estimate() resolves to dictionary with members');
> +
> +function deleteDB(name) {
> +  return new Promise(function(resolve, reject) {
> +    let del = indexedDB.deleteDatabase(name);

rename del to request or deleteRequest ?

@@ +25,5 @@
> +    del.onsuccess = function() { resolve(); };
> +  });
> +}
> +
> +function openDB(name, upgrade) {

Is it worth to have this function, given there's only one caller ?

@@ +27,5 @@
> +}
> +
> +function openDB(name, upgrade) {
> +  return new Promise(function(resolve, reject) {
> +    let open = indexedDB.open(name);

rename open to request or openRequest ?

@@ +38,5 @@
> +promise_test(function(t) {
> +  const arraySize = 1e6;
> +  const objectStoreName = "storagesManager";
> +  const dbname = this.window ? window.location.pathname :
> +  	   "estimate-worker.https.html";

Nit: add an empty line here

@@ +39,5 @@
> +  const arraySize = 1e6;
> +  const objectStoreName = "storagesManager";
> +  const dbname = this.window ? window.location.pathname :
> +  	   "estimate-worker.https.html";
> +  let db, before, usageAfterCreate, usageAfterPut;

Nit: give "db" its own separate line

rename "before" to "usageBeforeCreate" ?

@@ +46,5 @@
> +  .then(() => {
> +     return navigator.storage.estimate();
> +  })
> +  .then(estimate => {
> +    before = estimate.usage;

Can we open the db right here, instead of doing it in next "then" ?

@@ +56,5 @@
> +  })
> +  .then(estimate => {
> +    usageAfterCreate = estimate.usage;
> +    assert_greater_than(usageAfterCreate, before,
> +  	                'estimated usage should increase after object store created');

Nit: *is* created
Nit: add an empty line here

@@ +71,5 @@
> +  })
> +  .then(estimate => {
> +    usageAfterPut = estimate.usage;
> +    assert_greater_than(usageAfterPut, usageAfterCreate,
> +  	                'estimated usage should increase after large value is stored');

Nit: add an empty line here

Comment 83

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Jan Varga [:janv] from comment #82)
> Comment on attachment 8830181 [details] [diff] [review]
> Bug 1268804 Part 2: Add test cases for SecureContext
> 
> Review of attachment 8830181 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, but I'd like to see updated patch before landing
> 
> ::: dom/workers/test/mochitest.ini
> @@ +212,5 @@
> >  skip-if = toolkit == 'android' #bug 982828
> >  [test_webSocket_sharedWorker.html]
> >  skip-if = toolkit == 'android' #bug 982828
> >  [test_worker_interfaces.html]
> > +scheme=https
> 
> What about test_interfaces.html ?
> 
> Also, shouldn't we now check what's exposed for http and for https
> separately ?
I removed because Bug 1333084 already added scheme=https label, and it was reviewed by bz. So I think it's also accurate to add the rest of test cases. But yes, i will ask bz.
> I recommend to ask one of the DOM peers.

> @@ +19,5 @@
> > +}, 'estimate() resolves to dictionary with members');
> > +
> > +function deleteDB(name) {
> > +  return new Promise(function(resolve, reject) {
> > +    let del = indexedDB.deleteDatabase(name);
> 
> rename del to request or deleteRequest ?
Sure
> @@ +25,5 @@
> > +    del.onsuccess = function() { resolve(); };
> > +  });
> > +}
> > +
> > +function openDB(name, upgrade) {
> 
> Is it worth to have this function, given there's only one caller ?
Sure, I can remove this function.
> @@ +27,5 @@
> > +}
> > +
> > +function openDB(name, upgrade) {
> > +  return new Promise(function(resolve, reject) {
> > +    let open = indexedDB.open(name);
> 
> rename open to request or openRequest ?
Okay. 
> @@ +39,5 @@
> > +  const arraySize = 1e6;
> > +  const objectStoreName = "storagesManager";
> > +  const dbname = this.window ? window.location.pathname :
> > +  	   "estimate-worker.https.html";
> > +  let db, before, usageAfterCreate, usageAfterPut;
> 
> Nit: give "db" its own separate line
> 
> rename "before" to "usageBeforeCreate" ?
Okay. 
> @@ +46,5 @@
> > +  .then(() => {
> > +     return navigator.storage.estimate();
> > +  })
> > +  .then(estimate => {
> > +    before = estimate.usage;
> 
> Can we open the db right here, instead of doing it in next "then" ?
Sure I can modify it.

Comment 84

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 1: Enable SecureContext for Storage API, r=baku

Comment 85

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 2: Add test cases for SecureContext, r=janv

Comment 86

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8833902 [details] [diff] [review]
Bug 1268804 Part 2: Add test cases for SecureContext, r=janv

Hi bz,
Do you have any concern if I change scheme to https for test_navigator.html, test_worker_interfaces.html in dom/workers/test/mochitest.ini? StorageManager is exposed to worker. See: https://storage.spec.whatwg.org/#storagemanager

I saw you give r+ in Bug 1333084 for test_interface.html, now I guess it's fine to test https channel only.

Comment 87

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8833267 [details] [diff] [review]
Bug 1268804 Part 3: Test storage api in webextension

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

Hi Shawn,
I took a first look at this test and added a couple of comments related to some minor tweaks, 
and a couple of additional questions related to the "content script" scenario.

I'm adding Kris as r? on this patch, because I'd like to hear his opinion of the "content script" scenario 
and he can give the patch another look and give the final sign off to it once it is ready.

::: toolkit/components/extensions/test/mochitest/test_ext_storage_manager_capabilities.html
@@ +14,5 @@
> +"use strict";
> +
> +add_task(function* test_backgroundScript() {
> +  yield SpecialPowers.pushPrefEnv({
> +    "set": [["dom.storageManager.enabled", true],

I would prefer this formatted as:

  yield SpecialPowers.pushPrefEnv({
    "set": [
      ["...", true],
      ...
      ["...", true],
    ],
  });

(and also, the trailing commas are required by the eslint rules applied here)

@@ +34,5 @@
> +    browser.test.assertTrue('persisted' in navigator.storage, 'Has persisted function');
> +    browser.test.assertEq('function', typeof navigator.storage.persisted, 'persisted is function');
> +    browser.test.assertTrue(navigator.storage.persisted() instanceof Promise, 'persisted() returns a promise');
> +
> +    return browser.test.sendMessage("done");

you can use `browser.test.notifyPass("navigation_storage_api.done");` here and `yield extension.awaitFinish("navigation_storage_api.done");` at line 54

@@ +37,5 @@
> +
> +    return browser.test.sendMessage("done");
> +  }
> +
> +  let extension = ExtensionTestUtils.loadExtension({

This can be reduced to:

  function background() {
    ...
  }

  let extension = ExtensionTestUtils.loadExtension({
    background,
  });

@@ +62,5 @@
> +  yield waitForLoad(win);
> +
> +  function contentScript() {
> +    // Should not access storage api in non-secure context.
> +    browser.test.assertFalse(navigator.storage, 'Should not access storage api');

If I get this scenario correctly, navigator.storage is not available because the webpage is from an http url,
and so the result that we get is not exactly that the "content script doesn't have access to the navigator.storage API",
it looks more about "a page that is coming from the unsecure http protocol doesn't have access to the navigator.storage API", 
that I guess is a scenario already tested elsewhere.  

the navigator.storage API is available from the content script if the webpage is accessed over an https url, am I right?

is this the way that it should behave?

I'm wondering how chrome behaves in the same scenario, is the navigator.storage API available from the content script when the page cannot access it because is coming from an http url?

I would also like to hear Kris' opinion on this (and I would like him to take another look at this test for the final sign off on the patch).

Comment 88

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8833902 [details] [diff] [review]
Bug 1268804 Part 2: Add test cases for SecureContext, r=janv

Ideally we should be testing both ways.  I thought I said that already in one of the bugs on this, and that there was a followup filed to do that, but I'm not seeing it....

So specifically, we should pull all the script out of test_interfaces and test_worker_interfaces and test_navigator out into external scripts.  Then we should have two versions of each test, one loaded over https and one not, linking to said external scripts.  Then we should have the test assertions assert that some things are visible only in the secure context version.

Comment 89

[Kris Maglione [:kmag]]

Comment on attachment 8833267 [details] [diff] [review]
Bug 1268804 Part 3: Test storage api in webextension

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

This looks good, but I think we need tests for content scripts running in HTTPS content pages too.

::: toolkit/components/extensions/test/mochitest/test_ext_storage_manager_capabilities.html
@@ +14,5 @@
> +"use strict";
> +
> +add_task(function* test_backgroundScript() {
> +  yield SpecialPowers.pushPrefEnv({
> +    "set": [["dom.storageManager.enabled", true],

I'm OK with the current formatting, but agree about the commas.

Also, can you please move this to a separate setup() task? That's the pattern that most of our other tests currently follow.

@@ +20,5 @@
> +           ["dom.storageManager.prompt.testing.allow", true]]
> +  });
> +
> +  function backgroundScript() {
> +    browser.test.assertTrue(navigator.storage, 'Has storage api interface');

This works for now, but the `assertTrue` function is technically not supposed to accept object values, so please coerce this to a boolean.

@@ +21,5 @@
> +  });
> +
> +  function backgroundScript() {
> +    browser.test.assertTrue(navigator.storage, 'Has storage api interface');
> +    // test estimate

Nit: Please capitalize and add full stop. And a blank line before these comments would be nice.

@@ +22,5 @@
> +
> +  function backgroundScript() {
> +    browser.test.assertTrue(navigator.storage, 'Has storage api interface');
> +    // test estimate
> +    browser.test.assertTrue('estimate' in navigator.storage, 'Has estimate function');

Nit: Please use double quotes for all strings. ESLint will reject single-quoted strings.

@@ +51,5 @@
> +  });
> +
> +  yield extension.startup();
> +  yield extension.awaitMessage("done");
> +  yield SpecialPowers.popPrefEnv();

We shouldn't do this before the other tasks have run. It poisons the results.

@@ +62,5 @@
> +  yield waitForLoad(win);
> +
> +  function contentScript() {
> +    // Should not access storage api in non-secure context.
> +    browser.test.assertFalse(navigator.storage, 'Should not access storage api');

Storage APIs in content scripts are tied to the content page, so I think this is fine as is. We should test that this is available for content scripts in HTTPS content pages, though.

Comment 90

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #88)
> Comment on attachment 8833902 [details] [diff] [review]
> Bug 1268804 Part 2: Add test cases for SecureContext, r=janv 
> So specifically, we should pull all the script out of test_interfaces and
> test_worker_interfaces and test_navigator out into external scripts.  Then
> we should have two versions of each test, one loaded over https and one not,
> linking to said external scripts.  Then we should have the test assertions
> assert that some things are visible only in the secure context version.
Okay, I will create an another patch to handle test_interfaces, test_worker_interfaces and test_navigator.

Comment 91

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Luca Greco [:rpl] from comment #87)
> Comment on attachment 8833267 [details] [diff] [review]
> Bug 1268804 Part 3: Test storage api in webextension
> 
> Review of attachment 8833267 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Shawn,
> I took a first look at this test and added a couple of comments related to
> some minor tweaks, 
> and a couple of additional questions related to the "content script"
> scenario.
> 
> :::
> toolkit/components/extensions/test/mochitest/
> test_ext_storage_manager_capabilities.html
> @@ +14,5 @@
> I would prefer this formatted as:
> 
>   yield SpecialPowers.pushPrefEnv({
>     "set": [
>       ["...", true],
>       ...
>       ["...", true],
>     ],
>   });
> 
> (and also, the trailing commas are required by the eslint rules applied here)
I didn't know that, thanks for telling me.
> @@ +62,5 @@
> > +  yield waitForLoad(win);
> > +
> > +  function contentScript() {
> > +    // Should not access storage api in non-secure context.
> > +    browser.test.assertFalse(navigator.storage, 'Should not access storage api');
> 
> If I get this scenario correctly, navigator.storage is not available because
> the webpage is from an http url,
> and so the result that we get is not exactly that the "content script
> doesn't have access to the navigator.storage API",
> it looks more about "a page that is coming from the unsecure http protocol
> doesn't have access to the navigator.storage API", 
> that I guess is a scenario already tested elsewhere.  
> 
> the navigator.storage API is available from the content script if the
> webpage is accessed over an https url, am I right?
Yes, we added SecureContext annotation to Storage API, so security manager checks the top-level document is a secure context. Originally I thought by default content script is http only.
See: https://w3c.github.io/webappsec-secure-contexts/#examples-top-level
 
> I'm wondering how chrome behaves in the same scenario, is the
> navigator.storage API available from the content script when the page cannot
> access it because is coming from an http url?

https://w3c.github.io/webappsec-secure-contexts/#packaged-applications
"A user agent that support packaged applications MAY consider as "secure" specific URL schemes whose contents are authenticated by the user agent. For example, FirefoxOS application resources are referred to by a URL whose scheme component is app:. Likewise, Chrome’s extensions and apps live on chrome-extension: schemes. These could reasonably be considered trusted origins."

https://www.w3.org/TR/secure-contexts/#is-origin-trustworthy
"On the other hand, the user agent MAY choose to extend this trust to other, vendor-specific URL schemes like app: or chrome-extension: which it can determine a priori to be trusted (see §7.1 Packaged Applications for detail)."

And I saw chrome add chrome-extension to the white-list
https://cs.chromium.org/chromium/src/chrome/common/secure_origin_whitelist.cc?sq=package:chromium&dr=CSs&rcl=1478708199&l=32

void GetSchemesBypassingSecureContextCheckWhitelist(
    std::set<std::string>* schemes) {
  schemes->insert(extensions::kExtensionScheme);
}

But I'm not sure this whitelist applied to both background script and content script. I will find out later.

Comment 92

[Kris Maglione [:kmag]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #91)
> https://w3c.github.io/webappsec-secure-contexts/#packaged-applications
> "A user agent that support packaged applications MAY consider as "secure"
> specific URL schemes whose contents are authenticated by the user agent. For
> example, FirefoxOS application resources are referred to by a URL whose
> scheme component is app:. Likewise, Chrome’s extensions and apps live on
> chrome-extension: schemes. These could reasonably be considered trusted
> origins."
> 
> https://www.w3.org/TR/secure-contexts/#is-origin-trustworthy
> "On the other hand, the user agent MAY choose to extend this trust to other,
> vendor-specific URL schemes like app: or chrome-extension: which it can
> determine a priori to be trusted (see §7.1 Packaged Applications for
> detail)."

Yes, we decided that moz-extension: origins would qualify as secure (which is
why this API works in background pages). Content scripts, though, are a special case.

> But I'm not sure this whitelist applied to both background script and
> content script. I will find out later.

My suspicion, without having looked, is that SecureOrigin properties would
appear on XrayWrappers for a sandbox with a moz-extension: origin even if the
page has an http: origin. However, our sandboxes have expanded principals
which inherit from both the extension root URL and the content page URL, which
would presumably not qualify as a secure origin.

Comment 93

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #88)
> Comment on attachment 8833902 [details] [diff] [review]
> Bug 1268804 Part 2: Add test cases for SecureContext, r=janv
> 
> So specifically, we should pull all the script out of test_interfaces and
> test_worker_interfaces and test_navigator out into external scripts.  Then
> we should have two versions of each test, one loaded over https and one not,
> linking to said external scripts.  Then we should have the test assertions
> assert that some things are visible only in the secure context version.
I've opened bug 1338438 for this.

Comment 94

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 1: Enable SecureContext for Storage API, r=baku

Rebase patch.

Comment 95

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 2: Add test cases for SecureContext, r=janv

Rebase patch:
1. Move test_worker_interfaces/test_navigator changes to bug 1338438
2. Add https scheme to dom/quota/test/mochitest.ini instead of dom/indexedDB/test/mochitest.ini due to bug 1286717

Comment 96

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 3: Test storage api in webextension

Tested content script in https channel, interfaces can be found, but now I found failure on returning a Promise. :(

6 INFO TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_storage_manager_capabilities_secure.html | Has storage api interface 
7 INFO TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_storage_manager_capabilities_secure.html | Has estimate function 
8 INFO TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_storage_manager_capabilities_secure.html | estimate is function - Expected: function, Actual: function 
9 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_storage_manager_capabilities_secure.html | estimate returns a promise 
    contentScript@jar:file:///tmp/generated-extension-11.xpi!/content_script.js:7:5
    @jar:file:///tmp/generated-extension-11.xpi!/content_script.js:1:11
    
10 INFO TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_storage_manager_capabilities_secure.html | Has persist function 
11 INFO TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_storage_manager_capabilities_secure.html | persist is function - Expected: function, Actual: function 
12 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_storage_manager_capabilities_secure.html | persist() returns a promise 
    contentScript@jar:file:///tmp/generated-extension-11.xpi!/content_script.js:11:5
    @jar:file:///tmp/generated-extension-11.xpi!/content_script.js:1:11
    
13 INFO TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_storage_manager_capabilities_secure.html | Has persisted function 
14 INFO TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_storage_manager_capabilities_secure.html | persisted is function - Expected: function, Actual: function 
15 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_storage_manager_capabilities_secure.html | persisted() returns a promise 
    contentScript@jar:file:///tmp/generated-extension-11.xpi!/content_script.js:15:5
    @jar:file:///tmp/generated-extension-11.xpi!/content_script.js:1:11

Comment 97

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 2: Add test cases for SecureContext, r=janv

Comment 98

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 3: Test storage api in webextension

Comment 99

[Jan Varga [:janv]]

Comment on attachment 8835919 [details] [diff] [review]
Bug 1268804 Part 2: Add test cases for SecureContext, r=janv

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

::: testing/web-platform/tests/storage/storage-estimate-indexeddb.js
@@ +17,5 @@
> +    assert_equals(typeof result.quota, 'number');
> +  });
> +}, 'estimate() resolves to dictionary with members');
> +
> +function openDB(name, upgrade) {

>@@ +25,5 @@
>> +    del.onsuccess = function() { resolve(); };
>> +  });
>> +}
>> +
>> +function openDB(name, upgrade) {
>
>Is it worth to have this function, given there's only one caller ?

I'll let you figure out what's wrong here.
Did you do a self-review before submitting this patch ?

Comment 100

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Jan Varga [:janv] from comment #99)
> Comment on attachment 8835919 [details] [diff] [review]
> Bug 1268804 Part 2: Add test cases for SecureContext, r=janv
> 
> Review of attachment 8835919 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/web-platform/tests/storage/storage-estimate-indexeddb.js
> @@ +17,5 @@
> > +    assert_equals(typeof result.quota, 'number');
> > +  });
> > +}, 'estimate() resolves to dictionary with members');
> > +
> > +function openDB(name, upgrade) {
> 
> >@@ +25,5 @@
> >> +    del.onsuccess = function() { resolve(); };
> >> +  });
> >> +}
> >> +
> >> +function openDB(name, upgrade) {
> >
> >Is it worth to have this function, given there's only one caller ?
> 
> I'll let you figure out what's wrong here.
> Did you do a self-review before submitting this patch ?
Sorry, I forgot to delete |openDB()|. I did a self-review but still missed this part.

Comment 101

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 2: Add test cases for SecureContext, r=janv

Comment 102

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8835923 [details] [diff] [review]
Bug 1268804 Part 3: Test storage api in webextension

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

::: toolkit/components/extensions/test/mochitest/test_ext_storage_manager_capabilities_secure.html
@@ +32,5 @@
> +    browser.test.assertTrue(navigator.storage !== undefined, "Has storage api interface");
> +    // Test estimate.
> +    browser.test.assertTrue("estimate" in navigator.storage, "Has estimate function");
> +    browser.test.assertEq("function", typeof navigator.storage.estimate, "estimate is function");
> +    browser.test.assertTrue(navigator.storage.estimate() instanceof Promise, "estimate returns a promise");

Hmm. I still don't understand why all 'instance of Promise' assertions fail in the content script (through https channels) here.
Is there any restriction in the content script that I'm not aware of?
The strange thing is that I added these lines to do further test. I don't see error.

  navigator.storage.estimate().then(estimation => {
    browser.test.log("estimate usage: " + estimation.usage);                             
  });

Comment 103

[Kris Maglione [:kmag]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #102)
> > +    browser.test.assertTrue(navigator.storage !== undefined, "Has storage api interface");
> > +    // Test estimate.
> > +    browser.test.assertTrue("estimate" in navigator.storage, "Has estimate function");
> > +    browser.test.assertEq("function", typeof navigator.storage.estimate, "estimate is function");
> > +    browser.test.assertTrue(navigator.storage.estimate() instanceof Promise, "estimate returns a promise");
> 
> Hmm. I still don't understand why all 'instance of Promise' assertions fail
> in the content script (through https channels) here.
> Is there any restriction in the content script that I'm not aware of?
> The strange thing is that I added these lines to do further test. I don't
> see error.
> 
>   navigator.storage.estimate().then(estimation => {
>     browser.test.log("estimate usage: " + estimation.usage);                
> 
>   });

The promise that this function returns belongs to the content page, but the Promise constructor belongs to the content script sandbox. `instanceof window.Promise` should work.

Comment 104

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Kris Maglione [:kmag] from comment #103)
> (In reply to Shawn Huang [:shawq@njohnjr] from comment #102)
> 
> The promise that this function returns belongs to the content page, but the
> Promise constructor belongs to the content script sandbox. `instanceof
> window.Promise` should work.
Yes. It works. Thanks for the hint.

Comment 105

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 3: Test storage api in WebExtensions

Comment 106

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 3: Test storage api in webextension

I've fixed the problems that Luca and Kris addressed in Comment 87 and Comment 89.
test_ext_storage_manager_capabilities_secure.html is added for testing content script in https. Security manager checks top-level browsing context, so I have to create an another page.

Comment 107

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hi Jan,
Adding browser_permissionsPromptAllow.js, browser_permissionsPromptDeny.js in bug 1286717 now requires extra efforts to this bug since chrome browser test also requires to support scheme https for testing. However, it seems that adding annotation "scheme=https" to browser.ini doesn't work. I will figure out how to make browser config can also support https as soon as possible.

Comment 108

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 4: Add isSecureContext pref in test_interfaces for StorageManager

Comment 109

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8836477 [details] [diff] [review]
Bug 1268804 Part 4: Add isSecureContext pref in test_interfaces for StorageManager

This patch depends on bug 1338438.

Comment 110

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #107)
> Hi Jan,
> Adding browser_permissionsPromptAllow.js, browser_permissionsPromptDeny.js
> in bug 1286717 now requires extra efforts to this bug since chrome browser
> test also requires to support scheme https for testing. However, it seems
> that adding annotation "scheme=https" to browser.ini doesn't work. I will
> figure out how to make browser config can also support https as soon as
> possible.

I think this is not a problem. My statement is not correct. I think somehow I forgot this is the tab that I opened. So loading https://example.com/browser/dom/quota/test/browser_permissionsPrompt.html should be fine. I will modify it in bug 1286717.

Comment 111

[Kris Maglione [:kmag]]

Comment on attachment 8836444 [details] [diff] [review]
Bug 1268804 Part 3: Test storage api in webextension

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

r=me with nits addressed.

::: toolkit/components/extensions/test/mochitest/mochitest-common.ini
@@ +82,5 @@
>  [test_ext_storage_content.html]
>  [test_ext_storage_tab.html]
> +[test_ext_storage_manager_capabilities.html]
> +[test_ext_storage_manager_capabilities_secure.html]
> +scheme=https

This isn't necessary. We don't need the mochitest document to load from https, only the file_sample.html page. We can open the https: version of that from the test_ext_storage_manager_capabilities.html test.

::: toolkit/components/extensions/test/mochitest/test_ext_storage_manager_capabilities.html
@@ +25,5 @@
> +
> +add_task(function* test_backgroundScript() {
> +  function background() {
> +    browser.test.assertTrue(navigator.storage !== undefined, "Has storage api interface");
> +    // Test estimate.

Nit: Please add blank lines before each of these comments. Same for the content script tests.

@@ +56,5 @@
> +  yield waitForLoad(win);
> +
> +  function contentScript() {
> +    // Should not access storage api in non-secure context.
> +    browser.test.assertTrue(navigator.storage === undefined,

Please use assertEq for this.

@@ +57,5 @@
> +
> +  function contentScript() {
> +    // Should not access storage api in non-secure context.
> +    browser.test.assertTrue(navigator.storage === undefined,
> +		            "A page from the unsecure http protocol" +

Nit: Need a space before the closing quote.

@@ +81,5 @@
> +  yield extension.awaitMessage("done");
> +
> +  yield extension.unload();
> +  win.close();
> +  yield SpecialPowers.popPrefEnv();

Nit: Please move this to a separate cleanup task so we don't accidentally add more tasks after it.

::: toolkit/components/extensions/test/mochitest/test_ext_storage_manager_capabilities_secure.html
@@ +24,5 @@
> +});
> +
> +add_task(function* test_contentScriptSecure() {
> +  let win = window.open("file_sample.html");
> +  yield waitForLoad(win);

This isn't necessary. The content script will load when the window is ready, whether we wait for it here or not.

Comment 112

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 4: Add isSecureContext pref in test_interfaces for StorageManager

Fix:
dom/workers/test/test_navigator.html
dom/workers/test/test_worker_interfaces.html

Comment 113

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Kris Maglione [:kmag] from comment #111)
> Comment on attachment 8836444 [details] [diff] [review]
> Bug 1268804 Part 3: Test storage api in webextension
> 
> Review of attachment 8836444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits addressed.
> 
> ::: toolkit/components/extensions/test/mochitest/mochitest-common.ini
> @@ +82,5 @@
> >  [test_ext_storage_content.html]
> >  [test_ext_storage_tab.html]
> > +[test_ext_storage_manager_capabilities.html]
> > +[test_ext_storage_manager_capabilities_secure.html]
> > +scheme=https
> 
> This isn't necessary. We don't need the mochitest document to load from
> https, only the file_sample.html page. We can open the https: version of
> that from the test_ext_storage_manager_capabilities.html test.
Hi Kris,
Thanks for your quick response. :)
One thing I want to confirm again since I'm not familiar with the detail about content script.
I'm probably wrong here.

If we loaded mochitest in http and do:
let win = window.open("https://example.com/tests/toolkit/components/extensions/test/mochitest/file_sample.html");

I guess this should be "Example 4". see: [1]

"If a non-secure context opens https://example.com/ in a new window, then things are more complicated. The new window’s status depends on how it was opened. If the non-secure context can obtain a reference to the secure context, or vice-versa, then the new window is not a secure context."

This means that the following will both produce non-secure contexts:

<a href="https://example.com/" target="_blank">Link!</a>

<script>
  var w = window.open("https://example.com/");
</script>

So we still can't access storage api here. Maybe I'm wrong here, maybe content script is in the sandbox and wont' be restricted by this rule?

[1] https://w3c.github.io/webappsec-secure-contexts/#examples-top-level

Comment 114

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8836567 [details] [diff] [review]
Bug 1268804 Part 4: Add isSecureContext pref in test_interfaces for StorageManager

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

Hi bz,
This patch depends on bug 1338438 which splits tests over http and https. I added isSecureContext pref to test over https only.

Comment 115

[Kris Maglione [:kmag]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #113)
> "If a non-secure context opens https://example.com/ in a new window, then
> things are more complicated. The new window’s status depends on how it was
> opened. If the non-secure context can obtain a reference to the secure
> context, or vice-versa, then the new window is not a secure context."
> 
> This means that the following will both produce non-secure contexts:

Ah. That's unfortunate. In that case, please use `scheme = https` for
test_ext_storage_manager_capabilities.html, and use an explicit http: URL for
the insecure version rather than an explicit https: URL for the secure
version.

> So we still can't access storage api here. Maybe I'm wrong here, maybe
> content script is in the sandbox and wont' be restricted by this rule?

At the moment, we never mark sandbox globals as secure contexts, so access to
SecureContext properties depends solely on the status of the window the object
belongs to.

Comment 116

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8836567 [details] [diff] [review]
Bug 1268804 Part 4: Add isSecureContext pref in test_interfaces for StorageManager

r=me, but do make sure those followups from bug 1338438 about checking that isSecureContext has the expected value in the relevant workers happen.

Comment 117

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #116)
> Comment on attachment 8836567 [details] [diff] [review]
> Bug 1268804 Part 4: Add isSecureContext pref in test_interfaces for
> StorageManager
> 
> r=me, but do make sure those followups from bug 1338438 about checking that
> isSecureContext has the expected value in the relevant workers happen.

Thank you. I will file follow-up bug for workers script. test_worker_interfaces.js should be able to make assertions about secure context state based on where it got loaded from.

Comment 118

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Kris Maglione [:kmag] from comment #115)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #113)
> > "If a non-secure context opens https://example.com/ in a new window, then
> > things are more complicated. The new window’s status depends on how it was
> > opened. If the non-secure context can obtain a reference to the secure
> > context, or vice-versa, then the new window is not a secure context."
> > 
> > This means that the following will both produce non-secure contexts:
> 
> Ah. That's unfortunate. In that case, please use `scheme = https` for
> test_ext_storage_manager_capabilities.html, and use an explicit http: URL for
> the insecure version rather than an explicit https: URL for the secure
> version.
Thank you. That sounds a good idea. :)

Comment 119

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 3: Test storage api in WebExtensions, r=kmag

Comment 120

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 4: Add isSecureContext pref in test_interfaces for StorageManager, r=bz

Comment 121

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

The patchset are ready but we still need to wait for bug 1286717 got landed.

Comment 122

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

According to the current situation, there is a quite long dependency list of bug 1286717. It may take more than two weeks. Somehow, considering this bug would change things not in dom/indexeddb, dom/quota. I think it should be ok to land this without considering persisted()/persist() first. Then I will enable https for persisted()/persist() for bug 1286717.

Comment 123

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1947623396aa51f352d750510a03fdb9d52a769

Comment 124

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Try-server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9a2d36885b8fbc53093b4f5aef3cfe5f8fb31c0

Comment 125

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 1: Enable SecureContext for Storage API, r=baku

Rebase.

Comment 126

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 2: Add test cases for SecureContext, r=janv

Remove changes in dom/quota/mochitest.ini for unbinding persist/persisted api. Rebase MANIFEST.json.

Comment 127

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 3: Test storage api in WebExtensions, r=kmag

Remove persisted/persist test for unbinding dependency.

Comment 128

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #124)
> Try-server:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c9a2d36885b8fbc53093b4f5aef3cfe5f8fb31c0

09:46:54 INFO - TEST-UNEXPECTED-FAIL | dom/indexedDB/test/test_storage_manager_estimate.html | This test left a service worker registered without cleaning it up 

Hmm, I never saw this error before. I have to investigate further. The only thing I did is just load this test case in https mode.

Comment 129

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #128)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #124)
> > Try-server:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=c9a2d36885b8fbc53093b4f5aef3cfe5f8fb31c0
> 
> 09:46:54 INFO - TEST-UNEXPECTED-FAIL |
> dom/indexedDB/test/test_storage_manager_estimate.html | This test left a
> service worker registered without cleaning it up 
> 
> Hmm, I never saw this error before. I have to investigate further. The only
> thing I did is just load this test case in https mode.

I run this test locally and it can pass.

Comment 130

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 3: Test storage api in WebExtensions, r=kmag

Comment 131

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77f14c38fa89528a4d3cd4621bae3b301bfd9f71

Comment 132

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #131)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=77f14c38fa89528a4d3cd4621bae3b301bfd9f71

https://treeherder.mozilla.org/logviewer.html#?job_id=77896792&repo=try&lineNumber=8050

It's strange that I saw backgroundScript just finished and it starts to load contentScript, and immediately reports timeout.

[task 2017-02-16T11:39:49.734469Z] 11:39:49     INFO - SpawnTask.js | Entering test setup
[task 2017-02-16T11:39:49.736355Z] 11:39:49     INFO - SpawnTask.js | Leaving test setup
[task 2017-02-16T11:39:49.738197Z] 11:39:49     INFO - SpawnTask.js | Entering test test_backgroundScript
[task 2017-02-16T11:39:49.742136Z] 11:39:49     INFO - Extension loaded
[task 2017-02-16T11:39:49.743976Z] 11:39:49     INFO - Buffered messages logged at 11:34:53
[task 2017-02-16T11:39:49.745930Z] 11:39:49     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_storage_manager_capabilities.html | Has storage api interface 
[task 2017-02-16T11:39:49.747909Z] 11:39:49     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_storage_manager_capabilities.html | Has estimate function 
[task 2017-02-16T11:39:49.751302Z] 11:39:49     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_storage_manager_capabilities.html | estimate is function - Expected: function, Actual: function 
[task 2017-02-16T11:39:49.753247Z] 11:39:49     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_storage_manager_capabilities.html | estimate returns a promise 
[task 2017-02-16T11:39:49.755363Z] 11:39:49     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_storage_manager_capabilities.html | navigation_storage_api.done 
[task 2017-02-16T11:39:49.757357Z] 11:39:49     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_storage_manager_capabilities.html | test result correct 
[task 2017-02-16T11:39:49.759257Z] 11:39:49     INFO - SpawnTask.js | Leaving test test_backgroundScript
[task 2017-02-16T11:39:49.761060Z] 11:39:49     INFO - SpawnTask.js | Entering test test_contentScript
[task 2017-02-16T11:39:49.762802Z] 11:39:49     INFO - Buffered messages logged at 11:34:54
[task 2017-02-16T11:39:49.764548Z] 11:39:49     INFO - Extension loaded
[task 2017-02-16T11:39:49.766251Z] 11:39:49     INFO - Buffered messages finished
[task 2017-02-16T11:39:49.768206Z] 11:39:49     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_storage_manager_capabilities.html | Test timed out.

Comment 133

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

With command "mach mochitest toolkit/components/extensions/test/mochitest/test_ext_storage_manager_capabilities.html --repeat=2"
I can reproduce "timed out" issue locally with repeat=2 arguement.
I even removed all the code "browser.test.assert*" storage api, I can still hit the issue at the second loop. I ran the test one time, it seems always pass.

Comment 134

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hi Kris,
I've combined two test cases in one file. However, I tested on the latest mozilla-inbound, it seems easy to stuck with loading content script. Is there anything wrong in my test? Or can you suggest which part I should check first?

Comment 135

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hmm. I saw log "INFO Extension loaded", but the content script never run. I guess I have to dig into how content scripts get loaded.

Comment 136

[Kris Maglione [:kmag]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #134)
> Hi Kris,
> I've combined two test cases in one file. However, I tested on the latest
> mozilla-inbound, it seems easy to stuck with loading content script. Is
> there anything wrong in my test? Or can you suggest which part I should
> check first?

It looks like there's a race in the content script handling. Moving the window.open() call to after the extension.startup() call seems to fix it.

Comment 137

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Kris Maglione [:kmag] from comment #136)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #134)
> > Hi Kris,
> > I've combined two test cases in one file. However, I tested on the latest
> > mozilla-inbound, it seems easy to stuck with loading content script. Is
> > there anything wrong in my test? Or can you suggest which part I should
> > check first?
> 
> It looks like there's a race in the content script handling. Moving the
> window.open() call to after the extension.startup() call seems to fix it.

Yes. That can fix my problem. Thank you so much.

Comment 138

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 3: Test storage api in WebExtensions, r=kmag

Comment 139

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 2: Add test cases for SecureContext, r=janv

Comment 140

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa52cfeeebfdff7a6e3b6e4bc046c8d3d5a36ba1

Comment 141

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #140)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=aa52cfeeebfdff7a6e3b6e4bc046c8d3d5a36ba1


TEST-UNEXPECTED-FAIL | dom/indexedDB/test/test_storage_manager_estimate.html | This test left a service worker registered without cleaning it up 

[task 2017-02-20T16:12:45.038058Z] 16:12:45     INFO - Running test in a worker
[task 2017-02-20T16:12:45.039757Z] 16:12:45     INFO - Worker: loading ["https://example.com/tests/dom/indexedDB/test/unit/test_storage_manager_estimate.js"]
[task 2017-02-20T16:12:45.042077Z] 16:12:45     INFO - Worker: starting tests
[task 2017-02-20T16:12:45.043904Z] 16:12:45     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | Has estimate function 
[task 2017-02-20T16:12:45.045869Z] 16:12:45     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimate is function 
[task 2017-02-20T16:12:45.047839Z] 16:12:45     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimate() method exists and returns a Promise 
[task 2017-02-20T16:12:45.049922Z] 16:12:45     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimated usage must increase after createObjectStore 
[task 2017-02-20T16:12:45.056792Z] 16:12:45     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimated usage must increase after putting large object 
[task 2017-02-20T16:12:45.058916Z] 16:12:45     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | Worker finished 
[task 2017-02-20T16:12:45.060603Z] 16:12:45     INFO - Running test in main thread
[task 2017-02-20T16:12:45.062512Z] 16:12:45     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | Has estimate function 
[task 2017-02-20T16:12:45.064283Z] 16:12:45     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimate is function 
[task 2017-02-20T16:12:45.066068Z] 16:12:45     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimate() method exists and returns a Promise 
[task 2017-02-20T16:12:45.067784Z] 16:12:45     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimated usage must increase after createObjectStore 
[task 2017-02-20T16:12:45.069491Z] 16:12:45     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimated usage must increase after putting large object 
[task 2017-02-20T16:12:45.071121Z] 16:12:45     INFO - Buffered messages finished
[task 2017-02-20T16:12:45.072898Z] 16:12:45     INFO - TEST-UNEXPECTED-FAIL | dom/indexedDB/test/test_storage_manager_estimate.html | This test left a service worker registered without cleaning it up

Comment 142

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I don't think we register service worker in test_storage_manager_estimate.html.
Add more logs in ServiceWorker to see which test case did not unregister test cases.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff039aa03889f01c4b7da1cc9b341408c7025ccf

Comment 143

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #142)
> I don't think we register service worker in
> test_storage_manager_estimate.html.
> Add more logs in ServiceWorker to see which test case did not unregister
> test cases.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ff039aa03889f01c4b7da1cc9b341408c7025ccf
This time I don't see the same issue.

So sending to try-server again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f15aefbf4fedd4b25dc1af6208f37516ecb0a1ab

Comment 144

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #143)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #142)
> > I don't think we register service worker in
> > test_storage_manager_estimate.html.
> > Add more logs in ServiceWorker to see which test case did not unregister
> > test cases.
> > 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=ff039aa03889f01c4b7da1cc9b341408c7025ccf
> This time I don't see the same issue.
> 
> So sending to try-server again:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f15aefbf4fedd4b25dc1af6208f37516ecb0a1ab
So it looks like indexeddb test case test_serviceworker.html never unregister service worker.
But it's very strange why all other test cases after test_serviceworker.html won't be checked.

https://treeherder.mozilla.org/logviewer.html#?job_id=79044635&repo=try&lineNumber=9834

06:39:26     INFO - TEST-OK | dom/indexedDB/test/test_request_readyState.html | took 1804ms
06:39:26     INFO - TEST-START | dom/indexedDB/test/test_sandbox.html
06:39:27     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:27     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 0
06:39:27     INFO - MEMORY STAT | vsize 660MB | vsizeMaxContiguous 742MB | residentFast 151MB | heapAllocated 67MB
06:39:27     INFO - TEST-OK | dom/indexedDB/test/test_sandbox.html | took 785ms
06:39:27     INFO - TEST-START | dom/indexedDB/test/test_serviceworker.html
06:39:27     INFO - ######### [Storage]: function mozilla::dom::workers::ServiceWorkerManager::GetOrCreateJobQueue, key: http://mochi.test:8888
06:39:27     INFO -  ######### [Storage]: function mozilla::dom::workers::ServiceWorkerManager::GetOrCreateJobQueue, scope: http://mochi.test:8888/tests/dom/indexedDB/test/service_worker_client.html
06:39:28     INFO -  
06:39:28     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:28     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:28     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:28     INFO - MEMORY STAT | vsize 665MB | vsizeMaxContiguous 742MB | residentFast 155MB | heapAllocated 69MB
06:39:28     INFO - TEST-OK | dom/indexedDB/test/test_serviceworker.html | took 532ms
06:39:28     INFO - TEST-START | dom/indexedDB/test/test_setVersion.html
06:39:30     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:30     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:30     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:30     INFO - MEMORY STAT | vsize 661MB | vsizeMaxContiguous 742MB | residentFast 152MB | heapAllocated 70MB
06:39:30     INFO - TEST-OK | dom/indexedDB/test/test_setVersion.html | took 2664ms
06:39:30     INFO - TEST-START | dom/indexedDB/test/test_setVersion_abort.html
06:39:32     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:32     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:32     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:32     INFO - MEMORY STAT | vsize 662MB | vsizeMaxContiguous 742MB | residentFast 153MB | heapAllocated 71MB
06:39:32     INFO - TEST-OK | dom/indexedDB/test/test_setVersion_abort.html | took 1759ms
06:39:32     INFO - TEST-START | dom/indexedDB/test/test_setVersion_events.html
06:39:35     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:35     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:35     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:35     INFO - MEMORY STAT | vsize 660MB | vsizeMaxContiguous 742MB | residentFast 149MB | heapAllocated 68MB
06:39:35     INFO - TEST-OK | dom/indexedDB/test/test_setVersion_events.html | took 2735ms
06:39:35     INFO - TEST-START | dom/indexedDB/test/test_setVersion_exclusion.html
06:39:37     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:37     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:37     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:37     INFO - MEMORY STAT | vsize 660MB | vsizeMaxContiguous 742MB | residentFast 152MB | heapAllocated 71MB
06:39:37     INFO - TEST-OK | dom/indexedDB/test/test_setVersion_exclusion.html | took 2061ms
06:39:37     INFO - TEST-START | dom/indexedDB/test/test_setVersion_throw.html
06:39:39     INFO - JavaScript error: http://mochi.test:8888/tests/dom/indexedDB/test/unit/test_setVersion_throw.js, line 42: ReferenceError: trigger_js_exception_by_calling_a_nonexistent_function is not defined
06:39:39     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:39     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:39     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:39     INFO - MEMORY STAT | vsize 658MB | vsizeMaxContiguous 742MB | residentFast 149MB | heapAllocated 71MB
06:39:39     INFO - TEST-OK | dom/indexedDB/test/test_setVersion_throw.html | took 1705ms
06:39:39     INFO - TEST-START | dom/indexedDB/test/test_success_events_after_abort.html
06:39:41     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:41     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:41     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:41     INFO - MEMORY STAT | vsize 658MB | vsizeMaxContiguous 742MB | residentFast 148MB | heapAllocated 69MB
06:39:41     INFO - TEST-OK | dom/indexedDB/test/test_success_events_after_abort.html | took 1910ms
06:39:41     INFO - TEST-START | dom/indexedDB/test/test_table_locks.html
06:39:43     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:43     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:43     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:43     INFO - MEMORY STAT | vsize 658MB | vsizeMaxContiguous 742MB | residentFast 150MB | heapAllocated 74MB
06:39:43     INFO - TEST-OK | dom/indexedDB/test/test_table_locks.html | took 1987ms
06:39:43     INFO - TEST-START | dom/indexedDB/test/test_table_rollback.html
06:39:45     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:45     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:45     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:45     INFO - MEMORY STAT | vsize 661MB | vsizeMaxContiguous 742MB | residentFast 155MB | heapAllocated 78MB
06:39:45     INFO - TEST-OK | dom/indexedDB/test/test_table_rollback.html | took 2047ms
06:39:45     INFO - TEST-START | dom/indexedDB/test/test_third_party.html
06:39:47     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:47     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:47     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:47     INFO - MEMORY STAT | vsize 661MB | vsizeMaxContiguous 742MB | residentFast 161MB | heapAllocated 85MB
06:39:47     INFO - TEST-OK | dom/indexedDB/test/test_third_party.html | took 1927ms
06:39:47     INFO - TEST-START | dom/indexedDB/test/test_traffic_jam.html
06:39:49     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:49     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:49     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:49     INFO - MEMORY STAT | vsize 660MB | vsizeMaxContiguous 742MB | residentFast 158MB | heapAllocated 80MB
06:39:49     INFO - TEST-OK | dom/indexedDB/test/test_traffic_jam.html | took 2686ms
06:39:49     INFO - TEST-START | dom/indexedDB/test/test_transaction_abort.html
06:39:51     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:51     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:51     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:51     INFO - MEMORY STAT | vsize 660MB | vsizeMaxContiguous 742MB | residentFast 156MB | heapAllocated 78MB
06:39:51     INFO - TEST-OK | dom/indexedDB/test/test_transaction_abort.html | took 1805ms
06:39:51     INFO - TEST-START | dom/indexedDB/test/test_transaction_abort_hang.html
06:39:53     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:53     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:53     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:53     INFO - MEMORY STAT | vsize 664MB | vsizeMaxContiguous 742MB | residentFast 155MB | heapAllocated 73MB
06:39:53     INFO - TEST-OK | dom/indexedDB/test/test_transaction_abort_hang.html | took 1882ms
06:39:53     INFO - TEST-START | dom/indexedDB/test/test_transaction_duplicate_store_names.html
06:39:55     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:55     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:55     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:55     INFO - MEMORY STAT | vsize 662MB | vsizeMaxContiguous 742MB | residentFast 152MB | heapAllocated 73MB
06:39:55     INFO - TEST-OK | dom/indexedDB/test/test_transaction_duplicate_store_names.html | took 1894ms
06:39:55     INFO - TEST-START | dom/indexedDB/test/test_transaction_error.html
06:39:57     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:57     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:57     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:57     INFO - MEMORY STAT | vsize 662MB | vsizeMaxContiguous 742MB | residentFast 153MB | heapAllocated 75MB
06:39:57     INFO - TEST-OK | dom/indexedDB/test/test_transaction_error.html | took 1819ms
06:39:57     INFO - TEST-START | dom/indexedDB/test/test_transaction_lifetimes.html
06:39:59     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:39:59     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:39:59     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:39:59     INFO - MEMORY STAT | vsize 662MB | vsizeMaxContiguous 742MB | residentFast 150MB | heapAllocated 72MB
06:39:59     INFO - TEST-OK | dom/indexedDB/test/test_transaction_lifetimes.html | took 1854ms
06:39:59     INFO - TEST-START | dom/indexedDB/test/test_transaction_lifetimes_nested.html
06:40:00     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:40:00     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:40:00     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:40:00     INFO - MEMORY STAT | vsize 662MB | vsizeMaxContiguous 742MB | residentFast 150MB | heapAllocated 73MB
06:40:00     INFO - TEST-OK | dom/indexedDB/test/test_transaction_lifetimes_nested.html | took 872ms
06:40:00     INFO - TEST-START | dom/indexedDB/test/test_transaction_ordering.html
06:40:02     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:40:02     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:40:02     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:40:02     INFO - MEMORY STAT | vsize 662MB | vsizeMaxContiguous 742MB | residentFast 150MB | heapAllocated 75MB
06:40:02     INFO - TEST-OK | dom/indexedDB/test/test_transaction_ordering.html | took 1755ms
06:40:02     INFO - TEST-START | dom/indexedDB/test/test_unique_index_update.html
06:40:03     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:40:03     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:40:03     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:40:03     INFO - MEMORY STAT | vsize 663MB | vsizeMaxContiguous 742MB | residentFast 150MB | heapAllocated 75MB
06:40:03     INFO - TEST-OK | dom/indexedDB/test/test_unique_index_update.html | took 1826ms
06:40:03     INFO - TEST-START | dom/indexedDB/test/test_view_put_get_values.html
06:40:05     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:40:05     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:40:05     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:40:05     INFO - MEMORY STAT | vsize 663MB | vsizeMaxContiguous 742MB | residentFast 150MB | heapAllocated 78MB
06:40:05     INFO - TEST-OK | dom/indexedDB/test/test_view_put_get_values.html | took 1837ms
06:40:05     INFO - TEST-START | dom/indexedDB/test/test_wasm_cursors.html
06:40:07     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:40:07     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:40:07     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:40:07     INFO - MEMORY STAT | vsize 662MB | vsizeMaxContiguous 742MB | residentFast 146MB | heapAllocated 73MB
06:40:07     INFO - TEST-OK | dom/indexedDB/test/test_wasm_cursors.html | took 2138ms
06:40:07     INFO - TEST-START | dom/indexedDB/test/test_wasm_getAll.html
06:40:11     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:40:11     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:40:11     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:40:11     INFO - MEMORY STAT | vsize 659MB | vsizeMaxContiguous 742MB | residentFast 143MB | heapAllocated 74MB
06:40:11     INFO - TEST-OK | dom/indexedDB/test/test_wasm_getAll.html | took 3825ms
06:40:11     INFO - TEST-START | dom/indexedDB/test/test_wasm_index_getAllObjects.html
06:40:15     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:40:15     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:40:15     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:40:15     INFO - MEMORY STAT | vsize 660MB | vsizeMaxContiguous 742MB | residentFast 141MB | heapAllocated 71MB
06:40:15     INFO - TEST-OK | dom/indexedDB/test/test_wasm_index_getAllObjects.html | took 3930ms
06:40:15     INFO - TEST-START | dom/indexedDB/test/test_wasm_indexes.html
06:40:17     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:40:17     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:40:17     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:40:17     INFO - MEMORY STAT | vsize 659MB | vsizeMaxContiguous 742MB | residentFast 139MB | heapAllocated 69MB
06:40:17     INFO - TEST-OK | dom/indexedDB/test/test_wasm_indexes.html | took 2023ms
06:40:17     INFO - TEST-START | dom/indexedDB/test/test_wasm_put_get_values.html
06:40:19     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:40:19     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:40:19     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:40:19     INFO - MEMORY STAT | vsize 658MB | vsizeMaxContiguous 742MB | residentFast 140MB | heapAllocated 72MB
06:40:19     INFO - TEST-OK | dom/indexedDB/test/test_wasm_put_get_values.html | took 2012ms
06:40:19     INFO - TEST-START | Shutdown
06:40:19     INFO - Passed:  26789
06:40:19     INFO - Failed:  0
06:40:19     INFO - Todo:    64
06:40:19     INFO - Mode:    non-e10s
06:40:19     INFO - Slowest: 6921ms - /tests/dom/indexedDB/test/test_database_onclose.html
06:40:19     INFO - SimpleTest FINISHED
06:40:19     INFO - TEST-INFO | Ran 1 Loops
06:40:19     INFO - SimpleTest FINISHED
06:40:20     INFO - TEST-INFO | Main app process: exit 0
06:40:20     INFO - runtests.py | Application ran for: 0:04:31.824000
06:40:20     INFO - zombiecheck | Reading PID log: c:\users\cltbld\appdata\local\temp\tmpicwjghpidlog
06:40:20     INFO - ==> process 3840 launched child process 3664 ("c:\slave\test\build\application\firefox\firefox.exe" -contentproc --channel="3840.0.865087374\1005332274" -childID 1 -isForBrowser -intPrefs 5:50|6:-1|28:1000|33:0|34:0|43:128|44:10000|47:0|49:400|50:1|51:0|52:0|57:0|58:120|59:120|132:2|133:1|146:0|155:0|157:0|168:10000|180:-1|185:128|186:10000|187:0|193:24|194:32768|196:0|197:0|205:2|209:1048576|210:100|211:5000|213:600|215:1|224:2|229:0|239:60000| -boolPrefs 1:0|2:0|4:1|26:1|27:1|30:0|35:1|36:0|37:0|38:0|39:1|40:0|41:1|42:1|45:0|46:0|48:0|53:1|54:1|55:1|56:1|60:1|61:1|62:0|63:1|64:1|65:1|66:1|69:0|70:0|73:1|74:1|78:1|79:1|80:1|81:0|83:0|84:0|85:0|86:0|89:0|90:1|91:1|92:1|93:1|94:1|95:1|96:1|97:1|98:0|99:1|100:0|101:1|102:1|103:1|104:1|105:1|106:1|107:0|108:0|109:1|110:1|111:1|112:1|113:1|114:1|115:1|116:1|117:1|118:1|119:1|120:1|122:1|123:0|124:0|125:1|126:1|130:1|131:1|134:1|135:0|140:0|145:0|148:1|151:1|152:1|156:0|159:1|160:1|162:1|163:1|169:1|170:0|171:1|173:0|179:0|181:1|182:0|183:1|184:0|191:0|192:0|195:1|198:0|200:1|202:1|203:0|208:0|212:1|217:0|218:0|219:0|220:1|222:1|223:1|226:0|231:1|232:0|233:1|234:1|235:0|236:1|237:1|238:1|240:0|241:0|243:1|251:1|252:1|253:0|254:0|255:0| -stringPrefs "3:7:default|172:4:0.01|189:332: ����!
06:40:20     INFO - zombiecheck | Checking for orphan process with PID: 3664
06:40:20     INFO - runtests.py | Running with e10s: False
06:40:20     INFO - runtests.py | Running tests: start.
06:40:20     INFO - 
06:40:20     INFO - Application command: c:\slave\test\build\application\firefox\firefox.exe -marionette -foreground -profile c:\users\cltbld\appdata\local\temp\tmpa0jt05.mozrunner
06:40:20     INFO - runtests.py | Application pid: 2688
06:40:20     INFO - TEST-INFO | started process Main app process
06:40:21     INFO - 1487688021524	Marionette	INFO	Listening on port 2828
06:40:21     INFO - ######## [Storage]: mozilla::dom::workers::ServiceWorkerManager::AddScopeAndRegistration, key: http://mochi.test:8888
06:40:23     INFO - SimpleTest START
06:40:23     INFO - TEST-START | dom/indexedDB/test/test_storage_manager_estimate.html
06:40:26     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
06:40:26     INFO - #########[Storage] mozilla::dom::workers::ServiceWorkerManager::GetAllRegistrations, length: 1
06:40:26     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
06:40:26     INFO - TEST-INFO | started process screenshot
06:40:26     INFO - TEST-INFO | screenshot: exit 0
06:40:26     INFO - Buffered messages logged at 06:40:24
06:40:26     INFO - Running 'test_storage_manager_estimate.js'
06:40:26     INFO - Pushing preferences
06:40:26     INFO - Pushing permissions
06:40:26     INFO - Clearing old databases
06:40:26     INFO - Running test in a worker
06:40:26     INFO - Worker: loading ["https://example.com/tests/dom/indexedDB/test/unit/test_storage_manager_estimate.js"]
06:40:26     INFO - Worker: starting tests
06:40:26     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | Has estimate function 
06:40:26     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimate is function 
06:40:26     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimate() method exists and returns a Promise 
06:40:26     INFO - Buffered messages logged at 06:40:25
06:40:26     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimated usage must increase after createObjectStore 
06:40:26     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimated usage must increase after putting large object 
06:40:26     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | Worker finished 
06:40:26     INFO - Running test in main thread
06:40:26     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | Has estimate function 
06:40:26     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimate is function 
06:40:26     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimate() method exists and returns a Promise 
06:40:26     INFO - Buffered messages logged at 06:40:26
06:40:26     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimated usage must increase after createObjectStore 
06:40:26     INFO - TEST-PASS | dom/indexedDB/test/test_storage_manager_estimate.html | estimated usage must increase after putting large object 
06:40:26     INFO - Buffered messages finished
06:40:26     INFO - TEST-UNEXPECTED-FAIL | dom/indexedDB/test/test_storage_manager_estimate.html | This test left a service worker registered without cleaning it up 
06:40:26     INFO - afterCleanup@https://example.com/tests/SimpleTest/SimpleTest.js:1192:17
06:40:26     INFO - executeCleanupFunction@https://example.com/tests/SimpleTest/SimpleTest.js:1214:13
06:40:26     INFO - SimpleTest.finish@https://example.com/tests/SimpleTest/SimpleTest.js:1233:5
06:40:26     INFO - finishTest/</<@https://example.com/tests/dom/indexedDB/test/helpers.js:224:36
06:40:26     INFO - MEMORY STAT | vsize 579MB | vsizeMaxContiguous 740MB | residentFast 183MB | heapAllocated 91MB
06:40:26     INFO - TEST-OK | dom/indexedDB/test/test_storage_manager_estimate.html | took 3269ms
06:40:26     INFO - TEST-START | Shutdown
06:40:26     INFO - Passed:  11
06:40:26     INFO - Failed:  1
06:40:26     INFO - Todo:    0
06:40:26     INFO - Mode:    non-e10s

Comment 145

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hi Ehsan,
I saw you added code in SimpleTests to check service worker registered without cleaning it up.
So I wonder if you can help to comment this strange symptom.

test_serviceworker.html didn't unregister service worker, but the check did not assert that test case and wait for a while and assert in my test case, but my test case never registers service worker.
I checked all indexedDB test cases, only test_serviceworker.html registers service worker, so it matches the observation I saw in the log. After test_serviceworker.html registered service worker, |mRegistrationInfos.Count()| returns 1 in |ServiceWorkerManager::GetAllRegistrations|.

I suspect that SimpleTest._expectingRegisteredServiceWorker set to true. So other test cases after test_serviceworker.html won't be caught. Until my test case test_storage_manager_estimate.html loaded in https and it got caught.

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=79044635&repo=try&lineNumber=9834

Comment 146

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Sorry. I should look into service worker and investigate further. I have some ideas now.

Comment 147

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hmm.
How come this could run CreateNewRegistration before SimpleTest.Start?
But this is for sure related to test_serviceworker.html. I will dig into further more.
Maybe something is related to load my test case over https.

[task 2017-02-22T14:37:55.046648Z] 14:37:55     INFO - ==> process 1535 launched child process 1557
[task 2017-02-22T14:37:55.047873Z] 14:37:55     INFO - ==> process 1535 launched child process 1614
[task 2017-02-22T14:37:55.049850Z] 14:37:55     INFO - zombiecheck | Checking for orphan process with PID: 1557
[task 2017-02-22T14:37:55.052290Z] 14:37:55     INFO - zombiecheck | Checking for orphan process with PID: 1614
[task 2017-02-22T14:37:55.055268Z] 14:37:55     INFO - runtests.py | Running with e10s: False
[task 2017-02-22T14:37:55.059241Z] 14:37:55     INFO - runtests.py | Running tests: start.
[task 2017-02-22T14:37:55.061103Z] 14:37:55     INFO - 
[task 2017-02-22T14:37:55.063058Z] 14:37:55     INFO - Application command: /home/worker/workspace/build/application/firefox/firefox -marionette -foreground -profile /tmp/tmp80dhlT.mozrunner
[task 2017-02-22T14:37:55.112209Z] 14:37:55     INFO - runtests.py | Application pid: 1673
[task 2017-02-22T14:37:55.112651Z] 14:37:55     INFO - TEST-INFO | started process Main app process
[task 2017-02-22T14:37:57.934218Z] 14:37:57     INFO - 1487774277931	Marionette	INFO	Listening on port 2828
[task 2017-02-22T14:37:58.502452Z] 14:37:58     INFO - ### CreateNewRegistration
[task 2017-02-22T14:37:58.504571Z] 14:37:58     INFO - !!! AddScopeAndRegistration  !!!!!!!!!!!!!!!
[task 2017-02-22T14:37:58.504933Z] 14:37:58     INFO - ######## [Storage]: AddScopeAndRegistration, key: http://mochi.test:8888
[task 2017-02-22T14:38:02.500651Z] 14:38:02     INFO - SimpleTest START
[task 2017-02-22T14:38:02.508909Z] 14:38:02     INFO - TEST-START | dom/indexedDB/test/test_storage_manager_estimate.html
[task 2017-02-22T14:38:04.918543Z] 14:38:04     INFO - !!!!!!!!!!! SimpleTest._expectingRegisteredServiceWorker:  undefined
[task 2017-02-22T14:38:04.920379Z] 14:38:04     INFO - !!!!!!!!!!!!!!!!!!!!!!!!! GetAllRegistrations !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
[task 2017-02-22T14:38:04.921651Z] 14:38:04     INFO - #########[Storage] GetAllRegistrations, length: 1
[task 2017-02-22T14:38:04.923737Z] 14:38:04     INFO - #######[Storage] Service worker Key:http://mochi.test:8888
[task 2017-02-22T14:38:04.926429Z] 14:38:04     INFO - #######[Storage] Service worker ServiceWorkerRegistrationInfo: 1
[task 2017-02-22T14:38:04.928432Z] 14:38:04     INFO - !!!!!! ServiceWorkerRegistrationInfo: http://mochi.test:8888/tests/dom/indexedDB/test/service_worker_client.html

Comment 148

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

<bkelly> well, calling unregister() starts an asynchronous process
<bkelly> I think part of the problem here is that the test is not waiting for the service worker to fully activate
<bkelly> so when it calls unregister() its probably still activating
<bkelly> so the registration survives for a bit longer while that completes before the unregister runs
bkelly suggested to wait for service worker active in test_serviceworker.html then see if the problem is still there.

Comment 149

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #148)
> <bkelly> well, calling unregister() starts an asynchronous process
> <bkelly> I think part of the problem here is that the test is not waiting
> for the service worker to fully activate
> <bkelly> so when it calls unregister() its probably still activating
> <bkelly> so the registration survives for a bit longer while that completes
> before the unregister runs
> bkelly suggested to wait for service worker active in
> test_serviceworker.html then see if the problem is still there.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25e3bbf47fb71f554d9b0f781bee39a57893a8fc
Try to run the m-4 chunk for 85 times, it seems to work well.

Comment 150

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 5: Wait for ServiceWorker to be activated

Comment 151

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #149)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #148)
> > <bkelly> well, calling unregister() starts an asynchronous process
> > <bkelly> I think part of the problem here is that the test is not waiting
> > for the service worker to fully activate
> > <bkelly> so when it calls unregister() its probably still activating
> > <bkelly> so the registration survives for a bit longer while that completes
> > before the unregister runs
> > bkelly suggested to wait for service worker active in
> > test_serviceworker.html then see if the problem is still there.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=25e3bbf47fb71f554d9b0f781bee39a57893a8fc
> Try to run the m-4 chunk for 85 times, it seems to work well.

Remove all logs. Submit to try-server. I still can hit the error. :(
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa104f8a463d74fd9d012203ffb2424acfe4c765&selectedJob=79814649

Comment 152

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

[2017-02-24 14:29:15] <bkelly> if I am reading that log right its running your test in a separate browser instance from test_serviceworker.html
[2017-02-24 14:29:24] <bkelly> so its probably not the other test effecting you
[2017-02-24 14:32:20] <shawnjohnjr> But I dumped registration info
[2017-02-24 14:33:15] <shawnjohnjr> It's the same scope that test_serviceworker registered
[2017-02-24 14:34:22] <bkelly> well... different browser session means it must be coming from the serviceworker.txt file in the profile
[2017-02-24 14:38:07] <bkelly> there is probably a race where we shutdown the browser before we actually write out the serviceworkers.txt file and then your browser session starts and reloads the file
[2017-02-24 14:55:06] <bkelly> I think the right solution here may be to make mochitest create a new profile between the e10s and non-e10s tests here

Comment 153

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

So since test_storage_manager_estimate.html loads over https to fulfill SecureContext requirement. Testing framework will shutdown browser session over http, and starts an another session over https, and Service worker loads data from serviceworker.txt.

One thing I still need to confirm that if test_serviceworker.html unregisters itself, we don't update service worker registrar file as soon as possible? From the log [1], unregister from 06:11:09 and shutdown at 06:11:47, there are some others test cases running. We have 38 seconds to write data to serviceworker.txt, even if its an asynchronous process, i still have doubts.

If different browser sessions in different test runs are sharing a profile, shall we create different profiles for different browser sessions?

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=79557058&repo=try&lineNumber=3708

Comment 154

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=80019884
Log shows test_serviceworker.html writes things into serviceworker.txt when unregistering.
It looks like there is a race, normally we don't write suffix/scope into registrar file after unregister.
This is why when test framework starts new test case over https, it registers ServiceWorker.

Comment 155

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

TEST-UNEXPECTED-FAIL | /storage/estimate-indexeddb-worker.https.html | estimate() shows usage increase after large value is stored - assert_greater_than: estimated usage should increase after large value is stored expected a number greater than 759679 but got 225228

I still don't have any idea why estimate size reduces a lot. And this only appears on worker version with wpt test infrastructure.


https://treeherder.mozilla.org/#/jobs?repo=try&revision=b81c6059a9bbda07e46bedd35dc64f76c4a73877&selectedJob=79386298


[task 2017-02-22T15:34:08.935631Z] 15:34:08     INFO - TEST-START | /storage/estimate-indexeddb-worker.https.html
[task 2017-02-22T15:34:09.692360Z] 15:34:09     INFO - 
[task 2017-02-22T15:34:09.693486Z] 15:34:09     INFO - TEST-PASS | /storage/estimate-indexeddb-worker.https.html | estimate() method exists and returns a Promise - {}
[task 2017-02-22T15:34:09.694330Z] 15:34:09     INFO - {}
[task 2017-02-22T15:34:09.695460Z] 15:34:09     INFO - TEST-PASS | /storage/estimate-indexeddb-worker.https.html | estimate() resolves to dictionary with members - {}
[task 2017-02-22T15:34:09.696364Z] 15:34:09     INFO - {}
[task 2017-02-22T15:34:09.697604Z] 15:34:09     INFO - TEST-UNEXPECTED-FAIL | /storage/estimate-indexeddb-worker.https.html | estimate() shows usage increase after large value is stored - assert_greater_than: estimated usage should increase after large value is stored expected a number greater than 759679 but got 225228
[task 2017-02-22T15:34:09.698620Z] 15:34:09     INFO - @https://web-platform.test:8443/storage/storage-estimate-indexeddb.js:76:5
[task 2017-02-22T15:34:09.699775Z] 15:34:09     INFO - promise callback*@https://web-platform.test:8443/storage/storage-estimate-indexeddb.js:38:10
[task 2017-02-22T15:34:09.700345Z] 15:34:09     INFO - Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1409:20
[task 2017-02-22T15:34:09.700730Z] 15:34:09     INFO - promise callback*promise_test@https://web-platform.test:8443/resources/testharness.js:529:31
[task 2017-02-22T15:34:09.701263Z] 15:34:09     INFO - @https://web-platform.test:8443/storage/storage-estimate-indexeddb.js:21:1

Comment 156

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73b911536849ebece90dc70db11af1fac2d7b585
I've tried to wait for deleting indexeddb, it doesn't seem to solve the problem.
I will add more logs and check |MaybeUpdateSize|.

Comment 157

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Add more logs to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b3e2c06b2929b0f0149504a229ae63101fc6cfe

Comment 158

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #157)
> Add more logs to try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9b3e2c06b2929b0f0149504a229ae63101fc6cfe

The failure log shows very interesting:
https://public-artifacts.taskcluster.net/HBOh3WX-Q3mQ-JyHDSOZFQ/0/public/logs/live_backing.log

[task 2017-03-02T13:00:55.516741Z] 13:00:55     INFO - TEST-START | /storage/estimate-indexeddb-worker.https.html

We should expect QuotaObject::mPath should only be like /tmp/tmpzaclqA.mozrunner/storage/default/https+++web-platform.test+8443/idb/2650105373elsmttihm.astpet-twho.sqlite-wal.

When running estimate-indexeddb-worker.https.html, I should only see "idb" in mPath, but on the try-server wpt test case set 1, there are massive caches call MaybeUpdateSize.

And it looks like xWrite massively unstoppable call MaybeUpdateSize when estimate-indexeddb-worker.https.html is executing.


/tmp/tmpzaclqA.mozrunner/storage/default/https+++web-platform.test+8443/cache/caches.sqlite
[task 2017-03-02T13:00:58.096650Z] 13:00:58     INFO - PROCESS | 2277 | ### [Storage] xWrite Maybeupdate size: 40960
[task 2017-03-02T13:00:58.097097Z] 13:00:58     INFO - PROCESS | 2277 | ### [Storage] MaybeUpdateSize Maybeupdate size: 40960
[task 2017-03-02T13:00:58.097636Z] 13:00:58     INFO - PROCESS | 2277 | ### [Storage] MaybeUpdateSize Maybeupdate size: /tmp/tmpzaclqA.mozrunner/storage/default/https+++web-platform.test+8443/cache/caches.sqlite

xWrite is trying to Write data to a telemetry_file in "storage/TelemetryVFS.cpp".

And since both test cases are under the same origin (https://web-platform.test:8443), this is why we have unexpected size update.

So the chaos starts from "/service-workers/cache-storage/window/cache-add.https.html".

[task 2017-03-02T13:00:44.183513Z] 13:00:44     INFO - TEST-START | /service-workers/cache-storage/window/cache-add.https.html
[task 2017-03-02T13:00:44.191666Z] 13:00:44     INFO - Setting pref dom.caches.enabled (true)
[task 2017-03-02T13:00:44.718416Z] 13:00:44     INFO - PROCESS | 2277 | ### [Storage] xTruncate Maybeupdate size: 4096
[task 2017-03-02T13:00:44.718511Z] 13:00:44     INFO - PROCESS | 2277 | ### [Storage] MaybeUpdateSize Maybeupdate size: 4096
[task 2017-03-02T13:00:44.718603Z] 13:00:44     INFO - PROCESS | 2277 | ### [Storage] MaybeUpdateSize Maybeupdate size: /tmp/tmpzaclqA.mozrunner/storage/default/https+++web-platform.test+8443/cache/caches.sqlite


And we should expect it stops:
[task 2017-03-02T13:00:45.451845Z] 13:00:45     INFO - TEST-OK | /service-workers/cache-storage/window/cache-add.https.html | took 1262ms

Then starts with deletion tests:
[task 2017-03-02T13:00:45.461918Z] 13:00:45     INFO - TEST-START | /service-workers/cache-storage/window/cache-delete.https.html

Until the end of log, I still can see xWrite is keeping update size.

Comment 159

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hmm. I was wrong. Each time executeSql executed, xWrite would be called.

Comment 160

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

It would be interesting to know why xWrite executed so many times because of executeSql or not.

Comment 161

[Jan Varga [:janv]]

I think it's normal that DOM cache does some asynchronous writes after the test finishes. The problem here is that we don't clear origins between tests. Origin clearing would not only clear data but also wait for quota clients to finish asynchronous operations.

Comment 162

[Jan Varga [:janv]]

The other option is to move your test to a different origin somehow.

Comment 163

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Jan Varga [:janv] from comment #162)
> The other option is to move your test to a different origin somehow.

Changing a different origin is not enough, right. I have to change ETLD+1.

Comment 164

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

It looks like the current hostnames we supported only covers:
hostnames = ["web-platform.test",
             "www.web-platform.test",
             "www1.web-platform.test",
             "www2.web-platform.test",
             "xn--n8j6ds53lwwkrqhv28a.web-platform.test",
             "xn--lve-6lad.web-platform.test"]

http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/testing/web-platform/harness/wptrunner/environment.py#23

Comment 165

[Jan Varga [:janv]]

Ah, right.
However, we also have to keep in mind that these tests will possibly run in other browsers. Personally I think that each test should get "clean" storage area, so clearing sounds like a better option than changing origin.
The clearing should happen in the background w/o any explicit triggering from the tests, just like we do it for mochitests in dom/indexedDB
We don't have to add the clearing for all web-platform tests, just for service workers for now I think.

Comment 166

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #164)
> It looks like the current hostnames we supported only covers:
> hostnames = ["web-platform.test",
>              "www.web-platform.test",
>              "www1.web-platform.test",
>              "www2.web-platform.test",
>              "xn--n8j6ds53lwwkrqhv28a.web-platform.test",
>              "xn--lve-6lad.web-platform.test"]
> 
> http://searchfox.org/mozilla-central/rev/
> 546a05fec017cb785fca62a5199d42812a6a1fd3/testing/web-platform/harness/
> wptrunner/environment.py#23
Should be these domain names:
http://searchfox.org/mozilla-central/source/testing/web-platform/harness/wptrunner/browsers/server-locations.txt

Comment 167

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Jan Varga [:janv] from comment #165)
> Ah, right.
> However, we also have to keep in mind that these tests will possibly run in
> other browsers. Personally I think that each test should get "clean" storage
> area, so clearing sounds like a better option than changing origin.
> The clearing should happen in the background w/o any explicit triggering
> from the tests, just like we do it for mochitests in dom/indexedDB
> We don't have to add the clearing for all web-platform tests, just for
> service workers for now I think.
As far as I know, wpt test cases based on public DOM API.
I'm not sure we can clear storage in the background w/o any explicit triggering from DOM API.
What I did for mochitests, we still can use QMS to clear storage. Maybe I missed something.

Comment 168

[Jan Varga [:janv]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #167)
> As far as I know, wpt test cases based on public DOM API.
> I'm not sure we can clear storage in the background w/o any explicit
> triggering from DOM API.
> What I did for mochitests, we still can use QMS to clear storage. Maybe I
> missed something.

I haven't dug deep into wpt internals yet, but there's must be something that launches these tests. The launcher could also clear storage after a test finishes.

Comment 169

[Boris Zbarsky [:bzbarsky]]

> Changing a different origin is not enough, right. I have to change ETLD+1.

This recently came up as a problem for servo too.  We really need to get wpt adjusted so it offers hostnames from at least two different etld+1s.

Comment 170

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 2: Add test cases for SecureContext, r=janv

Split this patch (Part 2) into two patches. This one only changes mochitest tests to https. Another patch (Part 5) only addresses wpt test case.

Comment 171

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 5: Add wpt test cases for SecureContext, r=janv

Comment 172

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I guess it might take more time to adjust wpt test framework. So I want to land Part 1~4 first. Then I will land wpt test case after wpt test framework adjustment finished.

Comment 173

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd290ea533a608f78111d0cc3a827a9cabaf01e0

Comment 174

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/611a166e7b0c
Part 1: Enable SecureContext for Storage API, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3e5c9a2b190
Part 2: Add test cases for SecureContext, r=janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/a167a17bdcdf
Part 3: Test storage api in WebExtensions, r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/38911b466afc
Part 4: Add isSecureContext pref in test_interfaces for StorageManager, r=bz

Comment 175

[Jan Varga [:janv]]

Comment on attachment 8843892 [details] [diff] [review]
Bug 1268804 Part 5: Add wpt test cases for SecureContext, r=janv

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

::: testing/web-platform/tests/storage/storage-estimate-indexeddb.js
@@ +19,5 @@
> +}, 'estimate() resolves to dictionary with members');
> +
> +promise_test(function(t) {
> +  const arraySize = 1e6;
> +  const objectStoreName = "storagesManager";

Nit: "storageManager"

@@ +43,5 @@
> +    usageBeforeCreate = estimate.usage;
> +    return new Promise(function(resolve, reject) {
> +      let openRequest = indexedDB.open(dbname);
> +      openRequest.onerror = function() { reject(openRequest.error); };
> +      openRequest.onupgradeneeded = event => {

Nit: it's a bit weird that somewhere you use "function(args)" and elsewhere "args =>"

@@ +45,5 @@
> +      let openRequest = indexedDB.open(dbname);
> +      openRequest.onerror = function() { reject(openRequest.error); };
> +      openRequest.onupgradeneeded = event => {
> +        const db = event.target.result;
> +        db.createObjectStore(objectStoreName);

Nit: this can be just "openRequest.result.createObjectStore(objectStoreName);"

@@ +60,5 @@
> +    assert_greater_than(usageAfterCreate, usageBeforeCreate,
> +  	                'estimated usage should increase after object store is created');
> +
> +    let txn = db.transaction(objectStoreName, 'readwrite');
> +    txn.objectStore(objectStoreName).put(new Uint8Array(arraySize), 'k');

As I mentioned at the storage meeting, the array is 1MB, but it's all zeros by default, so IDB will shrink it during serialization of structured clone (we compress structured clones before storing on disk).
You can avoid the compression by storing a blob, or you can fill the array with random stuff.

Comment 176

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Jan Varga [:janv] from comment #175)
> Comment on attachment 8843892 [details] [diff] [review]
> Bug 1268804 Part 5: Add wpt test cases for SecureContext, r=janv
> 
> Review of attachment 8843892 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +60,5 @@
> > +    assert_greater_than(usageAfterCreate, usageBeforeCreate,
> > +  	                'estimated usage should increase after object store is created');
> > +
> > +    let txn = db.transaction(objectStoreName, 'readwrite');
> > +    txn.objectStore(objectStoreName).put(new Uint8Array(arraySize), 'k');
> 
> As I mentioned at the storage meeting, the array is 1MB, but it's all zeros
> by default, so IDB will shrink it during serialization of structured clone
> (we compress structured clones before storing on disk).
> You can avoid the compression by storing a blob, or you can fill the array
> with random stuff.
Somehow I thought our conclusion in the meeting is to wait for deleting database.
So I did not modify this part. I will store blob in the next revision.

Comment 177

[James Graham [:jgraham]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #169)
> > Changing a different origin is not enough, right. I have to change ETLD+1.
> 
> This recently came up as a problem for servo too.  We really need to get wpt
> adjusted so it offers hostnames from at least two different etld+1s.

Emailed public-test-infra about this; no objections so far so I guess I'll go ahead and implement soon. If you have any opinions on what the alternate domain should be called, now would be a good time to say (otherwise you will probably end up with web-platform-alternate.test)

Comment 178

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/611a166e7b0c
https://hg.mozilla.org/mozilla-central/rev/a3e5c9a2b190
https://hg.mozilla.org/mozilla-central/rev/a167a17bdcdf
https://hg.mozilla.org/mozilla-central/rev/38911b466afc

Comment 179

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I've discussed with Jan today, he said it's better to have wptrunner feature supporting "Clear" folders. Since we can't guarantee other test cases won't use the same ETLD+1 domain, it's possible that we still can't isolate each test cases. So I think I should check the possibility to support clearing origin for wptrunner.

Comment 180

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to James Graham [:jgraham] from comment #177)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #169)
> > > Changing a different origin is not enough, right. I have to change ETLD+1.
> > 
> > This recently came up as a problem for servo too.  We really need to get wpt
> > adjusted so it offers hostnames from at least two different etld+1s.
> 
> Emailed public-test-infra about this; no objections so far so I guess I'll
> go ahead and implement soon. If you have any opinions on what the alternate
> domain should be called, now would be a good time to say (otherwise you will
> probably end up with web-platform-alternate.test)

So does that mean we might only have one extra eTLD+1, what if other test cases still use this "web-platform-alternate.test" domain? It seems we can't guarantee other test cases won't use the same ETLD+1 domain.

Jan suggested we still need to figure out how to clear storage folders (under profile_folder/storage/default) for cache/storage api after finishing test cases if it's possible to change wptrunner. But I'm wondering if we can add code in testrunner.py for firefox specific, AFAIK, it's quite generic.

I guess the right place to do storage folder clean up is in test_ended [1] to isolate each test case.

[1] http://searchfox.org/mozilla-central/source/testing/web-platform/harness/wptrunner/testrunner.py#481

Comment 181

[James Graham [:jgraham]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #180)
> (In reply to James Graham [:jgraham] from comment #177)
> > (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #169)
> > > > Changing a different origin is not enough, right. I have to change ETLD+1.
> > > 
> > > This recently came up as a problem for servo too.  We really need to get wpt
> > > adjusted so it offers hostnames from at least two different etld+1s.
> > 
> > Emailed public-test-infra about this; no objections so far so I guess I'll
> > go ahead and implement soon. If you have any opinions on what the alternate
> > domain should be called, now would be a good time to say (otherwise you will
> > probably end up with web-platform-alternate.test)
> 
> So does that mean we might only have one extra eTLD+1, what if other test
> cases still use this "web-platform-alternate.test" domain? It seems we can't
> guarantee other test cases won't use the same ETLD+1 domain.

Yeah, we can't have a test-specific domain in the current system. I suggest we find a way to not need that.

> Jan suggested we still need to figure out how to clear storage folders
> (under profile_folder/storage/default) for cache/storage api after finishing
> test cases if it's possible to change wptrunner. But I'm wondering if we can
> add code in testrunner.py for firefox specific, AFAIK, it's quite generic.

If we need to do something firefox-specific and external to the browser we can do it in executormarionette.py (even if this means adding more plumbing to ensure it's called). What exact kind of per-test cleanup are you looking for in this case? Presumably something that can't be done from the test itself?

Comment 182

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to James Graham [:jgraham] from comment #181)
> What exact kind of per-test cleanup are you looking
> for in this case? Presumably something that can't be done from the test
> itself?
Yeah, it's supposed to cleanup from test itself.
We did cleanup[1] per origin by calling QuotaManager API in mochitest/xpcshell tests using SpecialPowers.

We need to cleanup directories under a user's Firefox profile 
<profile>/storage — the main top level directory for storages maintained by the quota manager (see below.)
<profile>/storage/permanent — persistent data storage repository
<profile>/storage/temporary — temporary data storage repository
<profile>/storage/default — default data storage repository

If I understand correctly, we should use standard DOM API in wpt tests.
So I think we can't do cleanup from the test itself.
That's why we're trying to figure out how we can cleanup from testrunner.

[1] http://searchfox.org/mozilla-central/source/dom/quota/test/unit/head.js#122

Comment 183

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Oh. I think now I know how to add cleanup code. Thanks, James.

Comment 184

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: (WIP) Bug 1268804 Part 6: Clear origin whenever loading new wpt test case

Comment 185

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: (WIP) Bug 1268804 Part 6: Clear origin whenever loading new wpt test case

Comment 186

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbc4d234ae864eef201fe6ffae49f90efa9b81fa

Comment 187

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #186)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=fbc4d234ae864eef201fe6ffae49f90efa9b81fa

Running wpt-1 for 91 times, it looks good now for clearing up.

Comment 188

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8848063 [details] [diff] [review]
(WIP) Bug 1268804 Part 6: Clear origin whenever loading new wpt test case

Move this patch to bug 1348214.

Comment 189

[Jan Varga [:janv]]

I have only one comment, if I'm reading the patch correctly, it clears storage for all tests. It would be more efficient to only do it for specific wpt modules, but I'm not an expert on wpt so I'll let wpt owner/reviewers to decide.

Comment 190

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Jan Varga [:janv] from comment #189)
> I have only one comment, if I'm reading the patch correctly, it clears
> storage for all tests. It would be more efficient to only do it for specific
> wpt modules, but I'm not an expert on wpt so I'll let wpt owner/reviewers to
> decide.
Yeah, I plan to clear storage only for storage test cases, maybe simply compare the url string.
Not sure what's the best way to do it.
After searching wptrunner, I don't see any folders specific check. I will ask James.
It's just a quick test on try for verifying WIP patch can fix the problem.
It's hard to reproduce the intermittent errors locally.

Comment 191

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1268804 Part 5: Add wpt test cases for SecureContext

Comment 192

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: interdiff for Part 5 wpt test

Comment 193

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=94f61772a415306635d5641178910ea58c6aeda0

Comment 194

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8849492 [details] [diff] [review]
Bug 1268804 Part 5: Add wpt test cases for SecureContext

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

I've fixed issues addressed in Comment 175, in addition to store blob into indexeddb.
I'm not sure it's ok to land this patch, or you have any additional comments.
So set review request again. I also upload the interdiff patch.

Bug 1348214 landed, so I guess this is the final step.

Comment 195

[Jan Varga [:janv]]

Comment on attachment 8849492 [details] [diff] [review]
Bug 1268804 Part 5: Add wpt test cases for SecureContext

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

Nice!

Comment 196

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f06ca112a30
Part 5: Add wpt test cases for SecureContext, r=janv

Comment 197

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f06ca112a30

Comment 198

[Chris Mills [:cmills]]

I've documented this by adding our new secure context banners and inline indicators into the API reference pages, as appropriate:

https://developer.mozilla.org/en-US/docs/Web/API/Storage_API
https://developer.mozilla.org/en-US/docs/Web/API/StorageManager (and child pages)
https://developer.mozilla.org/en-US/docs/Web/API/NavigatorStorage (and child pages)
https://developer.mozilla.org/en-US/docs/Web/API/StorageEstimate (and child pages)

I've also added a note to the Fx55 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/55#Security

Let me know if that looks OK. Thanks!