Mochitest coverage for the storage functionality - review

NEW
Assigned to

Status

()

Core
DOM: Quota Manager
P2
normal
6 months ago
4 months ago

People

(Reporter: Ioana Crisan, Assigned: Ioana Crisan)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 months ago
Please review the added coverage.
(Assignee)

Comment 1

6 months ago
Created attachment 8888259 [details] [diff] [review]
storage_test_coverage
(Assignee)

Comment 2

6 months ago
Run the tests locally and they pass.

Updated

6 months ago
Component: Integration: Mercurial → Storage
Product: MozReview → Toolkit

Updated

6 months ago
Component: Storage → DOM: Quota Manager
Product: Toolkit → Core

Comment 3

6 months ago
Correct the assignee.
Assignee: ttung → icrisan

Comment 4

6 months ago
Hi Ioana,

I'm afraid that I cannot review your patches. You need to send them to a Storage peer. However, I believe that I can give your feedback to accelerate the review process before your sending your patch to review.

Comment 5

6 months ago
So, before digging into your patch, I'd like to point out two things.

First of all, please follow the format (e.g. indentation) of other existing tests [1]. Moreover, we don't want to have trailing white space into our patch, and please remove them. If you are not sure about what to do, you could check this[2] out.

Secondly, If you want to put your tests under dom/quota, Jan is working on a new internal API (SimpleDB) in Bug 1361330. Rather than using IndexedDB to store data into the disk, I'd suggest using SimpleDB to store data into disk. 

For how to use this API, you could reference the dom/quota/test/unit/test_persist_groupLimit.js insides Bug 1361330 attachment 8882357 [details] [diff] [review].
Note: Bug 1361330 haven't been landed yet, so you need to apply patches there to your code base, and then you could use SimpleDB.

[1] http://searchfox.org/mozilla-central/source/dom/quota/test/browser_permissionsPrompt.html
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
Yeah, I can help with browser_* test first.
Comment on attachment 8888259 [details] [diff] [review]
storage_test_coverage

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

Thanks for your contribution, a lot of works. This is very initial feedback. I will check again after issues/questions addressed.

::: dom/quota/test/badEstimate1Bug1374970.html
@@ +12,5 @@
> +			// Set the global limit to 50MB
> +			SpecialPowers.pushPrefEnv({
> +				"set": [["dom.quotaManager.temporaryStorage.fixedLimit", 50000]]
> +			}, continueToNextStep);
> +			yield undefined;

Calling pushPrefEnv once is enough.

@@ +28,5 @@
> +				testGenerator.next(result.quota);
> +			})
> +			let groupLimit = yield undefined;
> +
> +			// Create an ArrayBuffer with the size of (groupLimit / 3)

Can you explain why you need to do "groupLimit / 3"?

::: dom/quota/test/badEstimate2Bug1374970.html
@@ +14,5 @@
> +				testGenerator.next(result.quota);
> +			})
> +			let groupLimit = yield undefined;
> +
> +			// Create an ArrayBuffer with the size of the (groupLimit / 2)  

Why do you need to do 'groupLimit /2'? Do you mind explaining it?

::: dom/quota/test/badEstimateBug1374970.js
@@ +25,5 @@
> +	info("Loading test page: " + testPageURL);
> +	gBrowser.selectedBrowser.loadURI(testPageURL);
> +	await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> +
> +	// Assert that the estimate() method returns the usage per origin and not per entire group 

trailing space

::: dom/quota/test/browserHelpers.js
@@ +47,5 @@
>      testGenerator.next();
>    }, 0);
>  }
> +
> +function openDB(dbname, objectStoreName) 

nit: trailing space

@@ +50,5 @@
> +
> +function openDB(dbname, objectStoreName) 
> +{
> +  return new Promise(function (resolve, reject) {
> +  let openRequest = indexedDB.open(dbname);

nit: fix indentation

::: dom/quota/test/browser_globalLimitValidation.js
@@ +93,5 @@
> +	info("Loading test page: " + testPageURL);
> +	gBrowser.selectedBrowser.loadURI(testPageURL);
> +	await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> +
> +	await waitForMessage('cleanup', gBrowser);

This doesn't seem to validate anything. Is it necessary to create a page to do cleanup?

::: dom/quota/test/browser_globalLimitValidation1.html
@@ +12,5 @@
> +      // Set the global limit to 50MB
> +      SpecialPowers.pushPrefEnv({
> +        "set": [["dom.quotaManager.temporaryStorage.fixedLimit", 50000]]
> +      }, continueToNextStep);
> +      yield undefined;

Calling pushPrefEnv once is enough.

@@ +20,5 @@
> +        ["dom.storageManager.prompt.testing", true],
> +        ["dom.storageManager.prompt.testing.allow", true],
> +        ["browser.storageManager.enabled", true]]
> +      }, continueToNextStep);
> +      yield undefined;

Maybe you can move SpecialPowers.pushPrefEnv() section to browser_globalLimitValidation.js? Since each browser_*.html use the same prefs.

@@ +22,5 @@
> +        ["browser.storageManager.enabled", true]]
> +      }, continueToNextStep);
> +      yield undefined;
> +
> +      // Get the tab's quota(groupLimit) 

trailing space

@@ +31,5 @@
> +
> +      navigator.storage.persist().then((result) => {
> +        testGenerator.next(result);
> +      })
> +      let persistResult = yield undefined;

persistResult doesn't seem to be used.

::: dom/quota/test/browser_globalLimitValidation2.html
@@ +2,5 @@
> +  Any copyright is dedicated to the Public Domain.
> +  http://creativecommons.org/publicdomain/zero/1.0/
> +-->
> +<html>
> + 

nit:trailing space. I won't point it out the rest of places.

::: dom/quota/test/browser_globalLimitValidation7.html
@@ +2,5 @@
> +  Any copyright is dedicated to the Public Domain.
> +  http://creativecommons.org/publicdomain/zero/1.0/
> +-->
> +<html>
> +

I'm not sure creating this page is necessary, maybe I missed something. Do you mind explaining the reason?

::: dom/quota/test/browser_privateWindowEstimate.js
@@ +13,5 @@
> +	win.gBrowser.selectedTab = win.gBrowser.addTab();
> +
> +	info("Loading test page: " + url);
> +	win.gBrowser.selectedBrowser.loadURI(url);
> +	await waitForMessage(10485760, win.gBrowser);

Just want to confirm that 10485760 constant value is the same even on different platform? Will it change on different platforms? Comparing this constant value seems to be dangerous. Since we run tests on different platforms.

::: dom/quota/test/browser_privateWindowPersisted.js
@@ +17,5 @@
> +	win.gBrowser.selectedBrowser.loadURI(testPageURL);
> +	await waitForMessage(false, win.gBrowser);
> +
> +	win.gBrowser.removeCurrentTab();
> +	win.close();

Please add one line after win.close():
win = null;
(Assignee)

Comment 8

6 months ago
(In reply to Tom Tung [:tt] from comment #5)

Hi Tom,

Thank you very much for your feedback. I will resolve the indentation and the trailing white spaces. Also I will change the way I store data into DB I will use simpleDB.
(Assignee)

Comment 9

6 months ago
(In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> Yeah, I can help with browser_* test first.

Hi Shawn,

Thank you very much for your review! I will reply your comment #7 after I will check the hard-coded values(e.g: "10485760" on all the platforms)
(In reply to Ioana Crisan from comment #9)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> > Yeah, I can help with browser_* test first.
> 
> Hi Shawn,
> 
> Thank you very much for your review! I will reply your comment #7 after I
> will check the hard-coded values(e.g: "10485760" on all the platforms)

Sorry, I did not explain the supported platforms very well.
See: https://mozilla-releng.net/trychooser/
This is try-server command generator. You can check which platform you need to test with.
Since there are many different platforms and I guess you can push your code to try server.
(Assignee)

Comment 11

6 months ago
(In reply to Shawn Huang [:shawnjohnjr] from comment #7)
> Comment on attachment 8888259 [details] [diff] [review]
> storage_test_coverage
> 
> Review of attachment 8888259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for your contribution, a lot of works. This is very initial feedback.
> I will check again after issues/questions addressed.
> 
> ::: dom/quota/test/badEstimate1Bug1374970.html
> @@ +12,5 @@
> > +			// Set the global limit to 50MB
> > +			SpecialPowers.pushPrefEnv({
> > +				"set": [["dom.quotaManager.temporaryStorage.fixedLimit", 50000]]
> > +			}, continueToNextStep);
> > +			yield undefined;
> 
> Calling pushPrefEnv once is enough.

Indeed, I will call the pushPrefEnv once.

> 
> @@ +28,5 @@
> > +				testGenerator.next(result.quota);
> > +			})
> > +			let groupLimit = yield undefined;
> > +
> > +			// Create an ArrayBuffer with the size of (groupLimit / 3)
> 
> Can you explain why you need to do "groupLimit / 3"?

In this test(badEstimate1Bug1374970.html) I wanted to test that estimate() method returns the usage per origin and not per entire group. 
The 2 sites used(belonging to the same group) would aggregately use a maximum of 20% of the global limit(one group limit). 
I started from setting the fixedLimit pref to 50MB, in this way the group limit will be 10MB(10,485,760 bytes).
Because both sites can aggregatly use 10MB(1,048,576 bytes) I tried to store different amount of data in each one to better differentiate what the estimate() method returns; creating a blob with the size of groupLimit/2 respectively groupLimit/3 is just an example, which summed up does not exceed the quota limit.
Because the estimate() method is run from the second html(where the blob stored is equal to groupLimit/2) I expected a quota of 5308544 bytes instead of 8869461 bytes(groupLimit/2 + groupLimit/3). This test is written based on the bug #1374970 

NOTE: This test is currently skipped until the bug is fixed

 
> ::: dom/quota/test/badEstimate2Bug1374970.html
> @@ +14,5 @@
> > +				testGenerator.next(result.quota);
> > +			})
> > +			let groupLimit = yield undefined;
> > +
> > +			// Create an ArrayBuffer with the size of the (groupLimit / 2)  
> 
> Why do you need to do 'groupLimit /2'? Do you mind explaining it?

Please see the above answer. Thanks!
 
> ::: dom/quota/test/badEstimateBug1374970.js
> @@ +25,5 @@
> > +	info("Loading test page: " + testPageURL);
> > +	gBrowser.selectedBrowser.loadURI(testPageURL);
> > +	await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> > +
> > +	// Assert that the estimate() method returns the usage per origin and not per entire group 
> 
> trailing space

I will fix the trailing spaces and also the indentation. Thanks!

> ::: dom/quota/test/browserHelpers.js
> @@ +47,5 @@
> >      testGenerator.next();
> >    }, 0);
> >  }
> > +
> > +function openDB(dbname, objectStoreName) 
> 
> nit: trailing space

I will fix the trailing spaces and also the indentation. Thanks!

> @@ +50,5 @@
> > +
> > +function openDB(dbname, objectStoreName) 
> > +{
> > +  return new Promise(function (resolve, reject) {
> > +  let openRequest = indexedDB.open(dbname);
> 
> nit: fix indentation

I will fix the trailing spaces and also the indentation. Thanks!

> ::: dom/quota/test/browser_globalLimitValidation.js
> @@ +93,5 @@
> > +	info("Loading test page: " + testPageURL);
> > +	gBrowser.selectedBrowser.loadURI(testPageURL);
> > +	await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> > +
> > +	await waitForMessage('cleanup', gBrowser);
> 
> This doesn't seem to validate anything. Is it necessary to create a page to
> do cleanup?

Yes, it is necessary, because otherwise the 'persistent-storage' permission is removed before the cleanup of the db takes place, meaning that the persisted data will not be affected by the cleanup, this will result in failing of the succeeded tests. So this is used to clear the db after the 'persistent-storage' permission is removed.
 
> ::: dom/quota/test/browser_globalLimitValidation1.html
> @@ +12,5 @@
> > +      // Set the global limit to 50MB
> > +      SpecialPowers.pushPrefEnv({
> > +        "set": [["dom.quotaManager.temporaryStorage.fixedLimit", 50000]]
> > +      }, continueToNextStep);
> > +      yield undefined;
> 
> Calling pushPrefEnv once is enough.

Indeed, I will call the pushPrefEnv once.

> @@ +20,5 @@
> > +        ["dom.storageManager.prompt.testing", true],
> > +        ["dom.storageManager.prompt.testing.allow", true],
> > +        ["browser.storageManager.enabled", true]]
> > +      }, continueToNextStep);
> > +      yield undefined;
> 
> Maybe you can move SpecialPowers.pushPrefEnv() section to
> browser_globalLimitValidation.js? Since each browser_*.html use the same
> prefs.

Yes, that seems to be working, should I do that for all the tests or just this one ?

> @@ +22,5 @@
> > +        ["browser.storageManager.enabled", true]]
> > +      }, continueToNextStep);
> > +      yield undefined;
> > +
> > +      // Get the tab's quota(groupLimit) 
> 
> trailing space

I will fix the trailing spaces and also the indentation. Thanks! :)

> @@ +31,5 @@
> > +
> > +      navigator.storage.persist().then((result) => {
> > +        testGenerator.next(result);
> > +      })
> > +      let persistResult = yield undefined;
> 
> persistResult doesn't seem to be used.

Yes, I will remove it.

> ::: dom/quota/test/browser_globalLimitValidation2.html
> @@ +2,5 @@
> > +  Any copyright is dedicated to the Public Domain.
> > +  http://creativecommons.org/publicdomain/zero/1.0/
> > +-->
> > +<html>
> > + 
> 
> nit:trailing space. I won't point it out the rest of places.

I will fix the trailing spaces and also the indentation. :)

> ::: dom/quota/test/browser_globalLimitValidation7.html
> @@ +2,5 @@
> > +  Any copyright is dedicated to the Public Domain.
> > +  http://creativecommons.org/publicdomain/zero/1.0/
> > +-->
> > +<html>
> > +
> 
> I'm not sure creating this page is necessary, maybe I missed something. Do
> you mind explaining the reason?

I responded to this to your comment above, it's necessary to do a proper cleanup of the db after the 'persistent-storage' permission has been removed. 

> ::: dom/quota/test/browser_privateWindowEstimate.js
> @@ +13,5 @@
> > +	win.gBrowser.selectedTab = win.gBrowser.addTab();
> > +
> > +	info("Loading test page: " + url);
> > +	win.gBrowser.selectedBrowser.loadURI(url);
> > +	await waitForMessage(10485760, win.gBrowser);
> 
> Just want to confirm that 10485760 constant value is the same even on
> different platform? Will it change on different platforms? Comparing this
> constant value seems to be dangerous. Since we run tests on different
> platforms.

Andrei will check this using the try-server you provided and will let us know. Thanks!

> ::: dom/quota/test/browser_privateWindowPersisted.js
> @@ +17,5 @@
> > +	win.gBrowser.selectedBrowser.loadURI(testPageURL);
> > +	await waitForMessage(false, win.gBrowser);
> > +
> > +	win.gBrowser.removeCurrentTab();
> > +	win.close();
> 
> Please add one line after win.close():
> win = null;

Sure, I will do.

Thanks a lot for the review!
> > ::: dom/quota/test/browser_globalLimitValidation.js
> > @@ +93,5 @@
> > > +	info("Loading test page: " + testPageURL);
> > > +	gBrowser.selectedBrowser.loadURI(testPageURL);
> > > +	await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> > > +
> > > +	await waitForMessage('cleanup', gBrowser);
> > 
> > This doesn't seem to validate anything. Is it necessary to create a page to
> > do cleanup?
> 
> Yes, it is necessary, because otherwise the 'persistent-storage' permission
> is removed before the cleanup of the db takes place, meaning that the
> persisted data will not be affected by the cleanup, this will result in
> failing of the succeeded tests. So this is used to clear the db after the
> 'persistent-storage' permission is removed.
>  
Maybe we can add finishTest() when onunload in browser_globalLimitValidation6.html 
<body onload="continueToNextStep()" onunload="finishTest()">
Then we don't need to create browser_globalLimitValidation7.html to do finishTest().

And 'persistent-storage' permission is for Storage API (which exposed to content page) not for qms.clearStoragesForPrincipal.
So calling qms.clearStoragesForPrincipal will clear the whole folder no matter it's persisted or best-effort.
(Assignee)

Comment 13

6 months ago
(In reply to Shawn Huang [:shawnjohnjr] from comment #12)
> > > ::: dom/quota/test/browser_globalLimitValidation.js
> > > @@ +93,5 @@
> > > > +	info("Loading test page: " + testPageURL);
> > > > +	gBrowser.selectedBrowser.loadURI(testPageURL);
> > > > +	await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> > > > +
> > > > +	await waitForMessage('cleanup', gBrowser);
> > > 
> > > This doesn't seem to validate anything. Is it necessary to create a page to
> > > do cleanup?
> > 
> > Yes, it is necessary, because otherwise the 'persistent-storage' permission
> > is removed before the cleanup of the db takes place, meaning that the
> > persisted data will not be affected by the cleanup, this will result in
> > failing of the succeeded tests. So this is used to clear the db after the
> > 'persistent-storage' permission is removed.
> >  
> Maybe we can add finishTest() when onunload in
> browser_globalLimitValidation6.html 
> <body onload="continueToNextStep()" onunload="finishTest()">
> Then we don't need to create browser_globalLimitValidation7.html to do
> finishTest().

I tried what you mention at the beginning but it doesn’t work that way, because when finishTest() will be called on onunload the site will still be persisted, meaning that the db won’t be cleared for the next test.

> And 'persistent-storage' permission is for Storage API (which exposed to
> content page) not for qms.clearStoragesForPrincipal.
> So calling qms.clearStoragesForPrincipal will clear the whole folder no
> matter it's persisted or best-effort.

Comment 14

6 months ago
Hi Ioana,

In today storage meeting with Jan, we discuss this. 

First of all, he wants to say thank you for doing this. :)
Also, you don't need to rewrite your patch to use SimpleDB, because it's an internal API and it's for simplifying complicated tests.
Flags: needinfo?(icrisan)
I ran the current patch through Try at Andrei's request. In the future, don't hesitate to needinfo me if you need help running your patch through Try until you get your SSH issues sorted out.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c4a6399e2df6c1d86074034d26eca9b2581a1a6
Looks like there's timeouts and debug assertions in the tests. The orange and red results in the previous link that don't have stars next to them show the issues that need attention. Also, please make sure the tests have trailing newlines at the end.
I also see this warning in the logs:
WARNING - Warning: badEstimateBug1374970.js from manifest /home/worker/workspace/build/tests/mochitest/browser/dom/quota/test/browser.ini is not a valid test
(Assignee)

Comment 18

5 months ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> I ran the current patch through Try at Andrei's request. In the future,
> don't hesitate to needinfo me if you need help running your patch through
> Try until you get your SSH issues sorted out.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5c4a6399e2df6c1d86074034d26eca9b2581a1a6

Thank you for running my patch through Try.
(Assignee)

Comment 19

5 months ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Looks like there's timeouts and debug assertions in the tests. The orange
> and red results in the previous link that don't have stars next to them show
> the issues that need attention. Also, please make sure the tests have
> trailing newlines at the end.

I will investigate the failing tests. I also created a new branch where I fixed the indentation, trailing spaces and the other small changes that Shawn requested me.
(Assignee)

Comment 20

5 months ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> I also see this warning in the logs:
> WARNING - Warning: badEstimateBug1374970.js from manifest
> /home/worker/workspace/build/tests/mochitest/browser/dom/quota/test/browser.
> ini is not a valid test

The badEstimateBug1374970.js is a test created for bug #1374970. It is not fixed yet and the test is skipped. If you want I can remove the test until the bug is fixed.
(Assignee)

Comment 21

5 months ago
Created attachment 8901804 [details] [diff] [review]
storage_test_coverage_2
Attachment #8888259 - Attachment is obsolete: true
Flags: needinfo?(icrisan)
(Assignee)

Comment 22

5 months ago
Hi Ryan,
Could you please run the newly patch through Try?
Thank you!
Flags: needinfo?(ryanvm)
Comment on attachment 8901804 [details] [diff] [review]
storage_test_coverage_2

Looks like this fixed the timeouts from the first patch :). A few comments:
1.) Looks like there's still a lot of missing newlines at the end of these files. Please make sure to add them (or configure your editor to do so automatically).
2.) Try to maintain alphabetical ordering in browser.ini instead of just adding the new tests to the end.
3.) The debug failures (like https://treeherder.mozilla.org/logviewer.html#?job_id=126449819&repo=try) are likely a real bug in Gecko code that will need attention from the developers of this feature.

Nice job, these are looking good!
Attachment #8901804 - Flags: feedback+
(Assignee)

Updated

5 months ago
Depends on: 1394745

Updated

5 months ago
No longer depends on: 1394745

Updated

5 months ago
Depends on: 1394745
(Assignee)

Comment 25

5 months ago
Created attachment 8902236 [details] [diff] [review]
storage_test_coverage_3

Created a patch(storage_test_coverage_3) and added newlines at the end of each file and also ordered alphabetically the tests in browser.ini. Patch will be run through Try after bug #1394745 is fixed.
I saw the patch title already came with r=shuang at the end. Unfortunately I'm not a peer to rubber-stamp patches. I commented patches because I just want to reduce unnecessary back-and-forth.
(Assignee)

Comment 27

5 months ago
(In reply to Shawn Huang [:shawnjohnjr] from comment #26)
> I saw the patch title already came with r=shuang at the end. Unfortunately
> I'm not a peer to rubber-stamp patches. I commented patches because I just
> want to reduce unnecessary back-and-forth.

Hi, Shawn
Thank you for helping me with the review! Could you please point me the person in charge for doing this?
Thank you!
I think you can ask Jan first. Let's see how we can continue this review process.
Flags: needinfo?(jvarga)

Comment 29

5 months ago
Yeah, I'll take a look at this, but maybe some time next week. I have many other reviews to do which waited for long time.
Flags: needinfo?(jvarga)

Updated

5 months ago
Attachment #8902236 - Flags: review?(jvarga)

Comment 30

5 months ago
Hi Ioana,

There is still a failure [1] after applying the fix in bug 1394745, but I think it's not related to the issue in bug 1394745. 
It looks like you need to handle the QuotaExceedError exception in the test.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=127050039&repo=try&lineNumber=32316
(Assignee)

Comment 31

5 months ago
(In reply to Tom Tung [:tt] from comment #30)
> Hi Ioana,
> 
> There is still a failure [1] after applying the fix in bug 1394745, but I
> think it's not related to the issue in bug 1394745. 
> It looks like you need to handle the QuotaExceedError exception in the test.
> 
> [1]
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=127050039&repo=try&lineNumber=32316

Hi Tom,

This happens only on Try. I cannot reproduce it on my desktop (ubuntu 16.04) :(

The failed test is the end to end scenario from the Etherpad: https://public.etherpad-mozilla.org/p/New_storage linked below:

[Steps]
Step 1. Open three sites in the different tabs. 
Step 2. Persist one of them. (e.g. persist a.com)
Step 3. Make Firefox reach the global limit by keeping storing data into that three sites.
Step 4. Close the tab to the other two sites. (e.g. close b.com & c.com)
Step 5. Try to write data into the persisted site, and it should work.
Step 6. Close the tab of persisted site and open the two sites which have been closed. (e.g. close a.com and open b.com & c.com)
Step 7. If you try to write more data into them, it should fail.

I didn't handle the "QuotaExceedError" exception when I added the first blob in the 3rd html because I didn't expect this to be triggered. I handled the exception when I added the second blob. 

I will investigate a bit more this issue.

Comment 32

5 months ago
(In reply to Ioana Crisan from comment #31)
> (In reply to Tom Tung [:tt] from comment #30) 
> This happens only on Try. I cannot reproduce it on my desktop (ubuntu 16.04)

BTW, the failure only happens on Window 7 debug build based in the try push [1]. Hope that hleps.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b39ee585cc5f8ef2a4fa26ec14513f3ae805bbcb&selectedJob=127050039
Which may be pointing at non-e10s mode as the issue. Try running the tests locally with --disable-e10s set and see what happens.
(Assignee)

Comment 34

5 months ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #33)
> Which may be pointing at non-e10s mode as the issue. Try running the tests
> locally with --disable-e10s set and see what happens.

If I run the tests locally without "--disable-e10s" I get the following results:

148 INFO Browser Chrome Test Summary
149 INFO Passed:  39
150 INFO Failed:  0
151 INFO Todo:    0
152 INFO Mode:    e10s


If I run the tests locally with "--disable-e10s" I get the following results:


Browser Chrome Test Summary
	Passed: 39
	Failed: 5
	Todo: 0
	Mode: non-e10s


The following tests failed:
152 INFO TEST-UNEXPECTED-FAIL | dom/quota/test/browser_globalLimitValidation.js | uncaught exception - QuotaExceededError at https://www.itisatrap.org/browser/dom/quota/test/browser_globalLimitValidation3.html:43:6
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1652
153 INFO TEST-UNEXPECTED-FAIL | dom/quota/test/browser_globalLimitValidation.js | uncaught exception - QuotaExceededError at https://example.org/browser/dom/quota/test/browser_globalLimitValidation5.html:36:6
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1652
154 INFO TEST-UNEXPECTED-FAIL | dom/quota/test/browser_globalLimitValidation.js | uncaught exception - QuotaExceededError at https://www.itisatrap.org/browser/dom/quota/test/browser_globalLimitValidation6.html:36:6
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1652
155 INFO TEST-UNEXPECTED-FAIL | dom/quota/test/browser_groupLimitNonPersistedOrigs.js | uncaught exception - QuotaExceededError at https://test2.example.com/browser/dom/quota/test/browser_groupLimitPersistedOrig2.html:51:6
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1652
156 INFO TEST-UNEXPECTED-FAIL | dom/quota/test/browser_groupLimitPersistedOrig.js | uncaught exception - QuotaExceededError at https://test2.example.com/browser/dom/quota/test/browser_groupLimitPersistedOrig2.html:51:6
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.