Closed Bug 1414755 Opened 2 years ago Closed 2 years ago

Get rid of ContentPrincipalInfoOriginNoSuffix

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(1 file)

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.
Attached patch principal.patchSplinter Review
Assignee: nobody → amarchesini
Attachment #8925446 - Flags: review?(bzbarsky)
I'm sorry for the lag here.  I'm not going to get to this until Monday (Nov 13)...
Priority: -- → P3
Comment on attachment 8925446 [details] [diff] [review]
principal.patch

Ben, do you mind double-checking the serviceworker tests?
Attachment #8925446 - Flags: review?(bkelly)
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...
Attachment #8925446 - Flags: review?(bzbarsky) → review+
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.
Attachment #8925446 - Flags: review?(bkelly) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f708f4650792
Get rid of ContentPrincipalInfoOriginNoSuffix, r=bz, r=bkelly
https://hg.mozilla.org/mozilla-central/rev/f708f4650792
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.