1358035 - Permaorange in test_persist.js when Gecko 55 merges to beta on 2017-06-12

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, enhancement)

Description

[Tom Tung [:tt, :ttung]]

As the description in summary, we did bad origin check when we are not in release and beta, and thus we should add prefs to both test_persist.js for preventing them from running in release and beta.

failure results: https://treeherder.mozilla.org/logviewer.html#?job_id=92721338&repo=try

Comment 1

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1358035: Move the check for persisting an invalid origin to test_bad_origin_directory.js and add skip beta/release flag on it since we don't check invaild origins on beta/release.

Review commit: https://reviewboard.mozilla.org/r/131956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131956/

Comment 2

[Jan Varga [:janv]]

Comment on attachment 8859902 [details]
Bug 1358035: Move the check for persisting an invalid origin to test_bad_origin_directory.js and add skip beta/release flag on it since we don't check invaild origins on beta/release.

https://reviewboard.mozilla.org/r/131956/#review134736

I think it would be better to just move the problematic part of the test to a new test, let's say test_bad_origin_directory.js (to be in sync with the IDB counterpart).
Otherwise, the rest of test_persist.js won't run at all.

Comment 3

[Tom Tung [:tt, :ttung]]

Comment on attachment 8859902 [details]
Bug 1358035: Move the check for persisting an invalid origin to test_bad_origin_directory.js and add skip beta/release flag on it since we don't check invaild origins on beta/release.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131956/diff/1-2/

Comment 4

[Jan Varga [:janv]]

Comment on attachment 8859902 [details]
Bug 1358035: Move the check for persisting an invalid origin to test_bad_origin_directory.js and add skip beta/release flag on it since we don't check invaild origins on beta/release.

https://reviewboard.mozilla.org/r/131956/#review134760

::: dom/quota/test/unit/test_bad_origin_directory.js:18
(Diff revision 2)
> +
> +  info("Persisting an invalid origin");
> +
> +  let invalidPrincipal = getPrincipal(invalidOrigin.url);
> +
> +  let request = persisted(invalidPrincipal, continueToNextStepSync);

info() says "Persisting ...", but you actually call persisted()

Where's the persist() call and where's the directory check - getRelativeFile(), etc. ?

Comment 5

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #4)
> Where's the persist() call and where's the directory check -
> getRelativeFile(), etc. ?

Sorry, I somehow deleted them...

Comment 6

[Tom Tung [:tt, :ttung]]

Comment on attachment 8859902 [details]
Bug 1358035: Move the check for persisting an invalid origin to test_bad_origin_directory.js and add skip beta/release flag on it since we don't check invaild origins on beta/release.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131956/diff/2-3/

Comment 7

[Jan Varga [:janv]]

Comment on attachment 8859902 [details]
Bug 1358035: Move the check for persisting an invalid origin to test_bad_origin_directory.js and add skip beta/release flag on it since we don't check invaild origins on beta/release.

https://reviewboard.mozilla.org/r/131956/#review134774

Looks good.

Comment 8

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #7)
> Looks good.

Thanks for the review!

Comment 9

[Tom Tung [:tt, :ttung]]

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

Comment 10

[Tom Tung [:tt, :ttung]]

[Tracking Requested - why for this release]:

Comment 11

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #10)
> [Tracking Requested - why for this release]:

Since test_persist.js failed in beta build, I would like to add tracking ff55 flag in this bug.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9faa446314c3
Move the check for persisting an invalid origin to test_bad_origin_directory.js and add skip beta/release flag on it since we don't check invaild origins on beta/release. r=janv

Comment 13

[Tom Tung [:tt, :ttung]]

Changed the bug summary since it cased Gecko 55 merges to beta failed.
Impact: the impact should be really small since I only created a test with skip beta/release flag and move problematic code in the test_persist.js into it.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9faa446314c3