1414755 - Get rid of ContentPrincipalInfoOriginNoSuffix

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Andrea Marchesini [:baku]]

Currently, we use ContentPrincipalInfoOriginNoSuffix because we were not able to generate the origin of a Principal URL out of the main-thread. In ServiceWorkerRegistrar, IndexedDB and nsJSPrincipals pass void_t as value of ContentPrincipalInfoOriginNoSuffix data struct.

This bug is about using MozURL to generate the origin URL where we are sure that the ContentPrincipalInfo URL has http/https/ftp schemes (ServiceWorkerRegistrar and IndexedDB) and passing the correct originNoSuffix in NSJSPrincipals serialization.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: principal.patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm sorry for the lag here.  I'm not going to get to this until Monday (Nov 13)...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8925446 [details] [diff] [review]
principal.patch

Ben, do you mind double-checking the serviceworker tests?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8925446 [details] [diff] [review]
principal.patch

> ReadPrincipalInfo(JSStructuredCloneReader* aReader,
>     } else if (aTag == SCTAG_DOM_NULL_PRINCIPAL) {
...
>+        nsAutoCString originNoSuffix;
>+        if (!ReadPrincipalInfo(aReader, attrs, spec, originNoSuffix)) {

Should we assert that originNoSuffix is empty here?  If it's _not_ empty, should it be passed to NullPrincipalInfo().  If not, then we should have a comment here explaining how things work.

>+++ b/dom/cache/DBSchema.cpp

Can we assert something about the URL scheme in ReadResponse?  If so, please do.

>+++ b/dom/workers/ServiceWorkerRegistrar.cpp
>+GetOrigin(const nsACString& aURL, nsACString& aOrigin)

This is the same code as in DBSchema, right?  Can we just put it somewhere shared?

Again, assert something about the URL scheme if possible.

In this file, there's a lot of duplicated code that starts with entry->scope(), extracts the origin, then creates a ContentPrincipalInfo.  For that matter, the attrs.PopulateFromSuffix(suffix) bit is duplicated too.  Could we factor all that out into a helper, please?

Is this data we're persisting somewhere?  Do we need to rev some versions somewhere, possibly?

>+++ b/ipc/glue/BackgroundUtils.cpp
>+        MOZ_CRASH("If the origin was in the contentPrincipalInfo, it must be available when deserialized");

We always have an origin in there now, right?  So there is no "If the origin was" about it...

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8925446 [details] [diff] [review]
principal.patch

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

I looked at DBSchema.cpp, ServiceWorkerRegistrar.cpp, and TestReadWrite.cpp.  The changes look reasonable to me.  I assume the test changes were due to ServiceWorkerRegistrar.cpp now actually requiring a parsable scope instead of treating it as an opaque token.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f708f4650792
Get rid of ContentPrincipalInfoOriginNoSuffix, r=bz, r=bkelly

Comment 7

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/f708f4650792