Phil Ringnalda (:philor) Reporter
	Description • 7 years ago

Because dom.storageManager.enabled defaults to false on release builds, you need to set it in test_storage_manager_persisted.js to avoid having test_storage_manager_persisted.html time out because 'navigator.storage is undefined' like it did in the merge simulation https://treeherder.mozilla.org/logviewer.html#?job_id=92721338&repo=try (you should be able to test locally by just changing the version number in /config/milestone.txt from 55a1 to 55).

[Tracking Requested - why for this release]:

Merge permaorange, closed tree, delayed b1, sadness.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor) Assignee
	Comment 1 • 7 years ago

Since file __dir__.ini was removed. Like Bug 1350732, we should set pref one by one.

	Phil Ringnalda (:philor) Reporter
	Comment 3 • 7 years ago

This was about the mochitest test_storage_manager_persisted.html, not those web-platform tests (there's a third storage wpt which will also fail, so this time I will add a __dir__.ini, which wasn't removed, it never existed).

	Tom Tung [:tt, :ttung]
	Comment 5 • 7 years ago

After discussing with Shawn, we found that test_persist.js [1] failed in this try push, either. Besides, this failure is not related to this bug.

We think the reason is that we protected origin check when it is not relase or beta builds [2]. Thus, we should add a pref to test_persist.js to ensure it not running in beta or release builds. I'll file a bug for it since this issue is not related this bug.


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9668396bb69fc202f0a7c1bbccdd9135818a40b&selectedJob=92728928
[2] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#1974-2003

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor) Assignee
	Comment 6 • 7 years ago

Comment on attachment 8859895 [details] [diff] [review]
Bug 1357975 - Set the pref to enable dom.storageManager for storage mochitests

I've modified version number config/milestone.txt from 55a1 to 55 and tested.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor) Assignee
	Comment 7 • 7 years ago

We also need to take care of browser tests. I will provide an another patch.

	Jan Varga [:janv]
	Comment 8 • 7 years ago

Comment on attachment 8859895 [details] [diff] [review]
Bug 1357975 - Set the pref to enable dom.storageManager for storage mochitests

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

::: dom/quota/test/helpers.js
@@ +47,5 @@
>  
>    info("Running" +
>         (testScriptFilename ? " '" + testScriptFilename + "'" : ""));
>  
> +  info("Pushing preferences");

Just to be consistent with dom/indexedDB/test/helpers.js, this should go before clearAllDatabases() call.

You can now remove the pref from test_storage_manager_persist_allow.js and test_storage_manager_persist_deny.js since it's set in helpers.js

	Jan Varga [:janv]
	Comment 10 • 7 years ago

Comment on attachment 8859905 [details] [diff] [review]
Bug 1357975 - Set the pref to enable dom.storageManager for storage mochitests

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

Nice

	Jan Varga [:janv]
	Comment 11 • 7 years ago

Comment on attachment 8859905 [details] [diff] [review]
Bug 1357975 - Set the pref to enable dom.storageManager for storage mochitests

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

Regarding the commit message, I think it would be better to say that we now set the pref in the helper, not in the tests.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor) Assignee
	Comment 12 • 7 years ago

(In reply to Jan Varga [:janv] from comment #11)
> Comment on attachment 8859905 [details] [diff] [review]
> Bug 1357975 - Set the pref to enable dom.storageManager for storage
> mochitests
> 
> Review of attachment 8859905 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Regarding the commit message, I think it would be better to say that we now
> set the pref in the helper, not in the tests.

I see. I will pay more attention to accuracy for commit messages next time.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor) Assignee
	Comment 14 • 7 years ago

Regarding browser test, we actually set pref in browser_permissionsPrompt.html.
So I think it's something else.
Looking into logs, I always saw "Failed to handle permission of type persistent-storage" in beta build.
Maybe it's related to frond-end changes. I will investigate a bit more.


GECKO(24449) | JavaScript error: file:///home/shawnjohnjr/code/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/bin/browser/components/nsBrowserGlue.js, line 2282: NS_ERROR_FAILURE: Failed to handle permission of type persistent-storage
6 INFO Console message: [JavaScript Error: "[Exception... "Failed to handle permission of type persistent-storage"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///home/shawnjohnjr/code/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/bin/browser/components/nsBrowserGlue.js :: prompt :: line 2282"  data: no]"]
prompt@file:///home/shawnjohnjr/code/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/bin/browser/components/nsBrowserGlue.js:2282:15

7 INFO Console message: [JavaScript Error: "NS_ERROR_FAILURE: Failed to handle permission of type persistent-storage" {file: "file:///home/shawnjohnjr/code/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/bin/browser/components/nsBrowserGlue.js" line: 2282}]

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor) Assignee
	Comment 15 • 7 years ago

We have to set pref "browser.storageManager.enabled" for enabling front-end features.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor) Assignee
	Comment 16 • 7 years ago

Front-end features are enabled only for nightly build. So to test prompts we have to enable pref no matter in beta.

	Jan Varga [:janv]
	Comment 17 • 7 years ago

Comment on attachment 8859930 [details] [diff] [review]
Bug 1357975 - Set pref browser.storageManager.enabled for storage browser tests

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

Well, we could do this in browserHelpers.js, but it's not urgent.

	Pulsebot
	Comment 18 • 7 years ago

Pushed by shuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a0c24d90fc4
Set the pref to enable dom.storageManager in helpers.js, r=janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/a485db46b96c
Set pref browser.storageManager.enabled for storage browser tests, r=janv

	Marcia Knous [:marcia]
	Comment 20 • 7 years ago

Tracking 55+ for this permaorange, especially to avoid delays in B1.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 21 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4a0c24d90fc4
https://hg.mozilla.org/mozilla-central/rev/a485db46b96c