1540247 - Expose MozURL::Init to JS

Status: RESOLVED WONTFIX

(Core :: Storage: Quota Manager, enhancement, P3)

Description

[Tom Tung [:tt, :ttung]]

See https://phabricator.services.mozilla.com/D25437#inline-146946

Comment 1

[Simon Giesecke [:sg] [he/him]]

The above-mentioned comment says:

I think we could do this safely under nsIQuotaManagerService as a boolean returning method.

So does this mean the actual resulting MozURL object can be discarded, and it should only be returned if the URL could be parsed without an error?

In that case,

  [must_use] nsIQuotaRequest isValidURL(in ACString aSpec);

should be added to dom/quota/nsIQuotaManagerService.idl? Do we need an optional baseURL here?

Comment 2

[Tom Tung [:tt, :ttung]]

(In reply to Simon Giesecke [:sg] from comment #1)

The above-mentioned comment says:

I think we could do this safely under nsIQuotaManagerService as a boolean returning method.

So does this mean the actual resulting MozURL object can be discarded, and it should only be returned if the URL could be parsed without an error?

Yes, the resulting MozURL object can be discarded. The callback function [1] should be called when it finished the check. So that caller can understand whether the URL is valid by the nsresult. (An assertion to ensure the returning argument URL for MozURL::Init() should not be a nullptr while the MozURL::Init() returns NS_OK is probably needed)
[1] https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/dom/quota/nsIQuotaRequests.idl#45

In that case,

  [must_use] nsIQuotaRequest isValidURL(in ACString aSpec);

For this use case, I think this is good enough. I would suggest having a comment above mentioning that the given URL is checked by MozURL::Init() or change the function name to something like isValidMozURL(). So that it would be more clear that it's checked by that method.

should be added to dom/quota/nsIQuotaManagerService.idl? Do we need an optional baseURL here?

We can expose that the optional argument baseURL when we needed in the future.

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1540247 - Expose MozURL::Init to JS. r=ttung

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cbce223800c1
Expose MozURL::Init to JS for QuotaManager tests r=ttung,asuth

Comment 5

[Raul Gurzau (:RaulG)]

Backed out changeset cbce223800c1 (bug 1540247) for xpcshell failures at test_obsoleteOrigins.js on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/67bb50bd8b67089c001ade51d157ad8e085f3e6c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=cbce223800c16c143e396a4f7cc82e57784fbc81&selectedJob=263890219

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=263890219&repo=autoland&lineNumber=2608

Log snippet:
[task 2019-08-28T16:38:24.642Z] 16:38:24 INFO - TEST-START | dom/push/test/xpcshell/test_clearAll_successful.js
[task 2019-08-28T16:38:26.204Z] 16:38:26 INFO - TEST-PASS | dom/push/test/xpcshell/test_clearAll_successful.js | took 1558ms
[task 2019-08-28T16:38:26.204Z] 16:38:26 INFO - Retrying tests that failed when run in parallel.
[task 2019-08-28T16:38:26.208Z] 16:38:26 INFO - TEST-START | dom/quota/test/unit/test_obsoleteOrigins.js
[task 2019-08-28T16:38:26.507Z] 16:38:26 WARNING - TEST-UNEXPECTED-FAIL | dom/quota/test/unit/test_obsoleteOrigins.js | xpcshell return code: 0
[task 2019-08-28T16:38:26.507Z] 16:38:26 INFO - TEST-INFO took 291ms
[task 2019-08-28T16:38:26.507Z] 16:38:26 INFO - >>>>>>>
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - TEST-PASS | dom/quota/test/unit/test_obsoleteOrigins.js | undefined assertion name - There should be a testSteps function - true == true
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - running event loop
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - dom/quota/test/unit/test_obsoleteOrigins.js | Starting testSteps
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - (xpcshell/head.js) | test testSteps pending (2)
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - "Verifying the obsolete origins are removed after the 2_1To2_2 upgrade and invalid origins wouldn't block upgrades"
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - "Upgrading"
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "Quota 'invalid' is not a valid scheme!: ActorsParent.cpp:10376"]"
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "Quota Origin 'invalid+++example.com' failed to parse, handled tokens: : ActorsParent.cpp:10282"]"
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - "Verifying the obsolete origins are removed after the 2_1To2_2 upgrade"
[task 2019-08-28T16:38:26.508Z] 16:38:26 WARNING - TEST-UNEXPECTED-FAIL | dom/quota/test/unit/test_obsoleteOrigins.js | testSteps - [testSteps : 61] Obsolete origin was removed during origin initialization - false == true
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - /builds/worker/workspace/build/tests/xpcshell/tests/dom/quota/test/unit/test_obsoleteOrigins.js:testSteps:61
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - exiting test
[task 2019-08-28T16:38:26.508Z] 16:38:26 INFO - Unexpected exception NS_ERROR_ABORT:

Comment 6

[Simon Giesecke [:sg] [he/him]]

When I implemented this, isValidMozURL("https://smaug----.github.io") was failing, but apparently that's no longer the case, probably due to the resolution of Bug 1568540. So, now "storage/default/https+++smaug----.github.io/" is included in the list of obsoleteOriginPaths in dom/quota/test/unit/test_obsoleteOrigins.js, but apparently the behaviour is not as expected. I am not familiar with this right now to judge where the problem is, either in the test or the tested code here. I need some advice on that, or maybe this follow-up work should be redirected to someone else.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Ah, sorry, I should have paid more attention in the review. https://smaug----.github.io is now valid and should not be obsolete. I think the comment was actually trying to express this, but it was a little ambiguous. Going from foggy memory (although there is a paper trail if anyone wants to check), I think the idea was that we wanted to codify that rust-url thought it was invalid for now, but eventually it would become valid. But we couldn't codify that because LocalStorage NextGen under debug builds checked for consistency between nsStandardURL and rust-url and would assert.

So I think what we want to do is:

• Remove the smaug domain from the obsolete origin test.
• Add it to the https://searchfox.org/mozilla-central/source/dom/quota/test/unit/test_validOrigins.js test.

Comment 8

[Simon Giesecke [:sg] [he/him]]

Ah, that makes sense! I thought the comment was referring to the reason why the line was commented out, but in fact in referred to the reason the line was actually in that test!

I will move it to test_validOrigins.js as you suggest.

However... this means that we don't need the newly exposed function at all right now, or am I mistaken on that? Should we keep it anyway?

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

That's a very good question. I'm a little confused as to where we stand after https://groups.google.com/forum/#!topic/mozilla.dev.platform/ILkf8vTRZsI announcing the resolution of bug 922464, in theory we can clean up QM and LSNG's handling of URL's further. But until we do that, the method might be useful because the code-path we're using in QM/LSNG is explicitly MozURL...

Comment 10

[Jan Varga [:janv]]

Yes, it seems, we don't need isValidMozURL(), at least for now. Regarding more cleanup, we use MozURL primarily to get the origin string and base domain and that can't be done with nsIURI/nsIPrincipal off main thread AFAIK.

Comment 11

[Simon Giesecke [:sg] [he/him]]

I removed the changes to test_obsoleteOrigins.js from the patch now, and opened Bug 1578146 to fix the test.

It is still not clear to be if we should land the patch exposing MozURL::Init, or if this should be closed as WONTFIX for now.

Comment 12

[Jan Varga [:janv]]

Let's not add a new nsIQuotaManagerService method if there's no use for it.