1209263 - Tell SpiderMonkey how to structured clone JSPrincipals

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Allow embedders to tell SpiderMonkey how to structured clone principals

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

bz, if you aren't the best reviewer for this, please forward the request. Thanks!

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8666946 [details] [diff] [review]
Allow embedders to tell SpiderMonkey how to structured clone principals

Looks like some of this code changed recently. Rebasing.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Allow embedders to tell SpiderMonkey how to structured clone principals

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8666968 [details] [diff] [review]
Allow embedders to tell SpiderMonkey how to structured clone principals

>+nsJSPrincipals::FinishRead(JSContext* aCx, JSStructuredCloneReader* aReader,

Maybe call this ReadKnownPrincipalType or something to indicate what it really does?

>+                           uint32_t aTag, uint32_t aAppId,

aAppId is unused.  Why is it there?

>+        if (!JS_ReadUint32Pair(aReader, &suffixLength, &specLength)) {
>+            return nullptr;

This function claims to return bool.

>+        if (!JS_ReadBytes(aReader, suffix.BeginWriting(), suffixLength)) {
>+            return nullptr;

And here.

>+        if (!JS_ReadBytes(aReader, spec.BeginWriting(), specLength)) {
>+            return nullptr;

And here.

>+    *aOutPrincipals = get(prin);

What keeps the object alive when "prin" comes off the stack?  Seems like this should be an XPCOM-style outparam, where the callee addrefs it and the caller is responsible for releasing or something.

>+    nsCOMPtr<nsIPrincipal> prin = PrincipalInfoToPrincipal(info, &rv);

Why does that not need either a 'using' declaration or more namespacing?

>+    return JS_WriteUint32Pair(aWriter, SCTAG_DOM_CONTENT_PRINCIPAL, 0) &&

If it's always 0, why are we claiming it's an appid during deserialization?

>+      static_assert(kSQLiteSchemaVersion == int32_t((22 << 4) + 0),

This needs review from someone with idb knowledge, but seems fishy at first glance.  We're changing kSQLiteSchemaVersion but not providing any migration functionality.

>+    MOZ_ASSERT(false, "not implemented");

Any reason to not make that a MOZ_CRASH for the worker case?

> The caller is responsible for calling
> + * JS_HoldPrincipals on the resulting JSPrincipals instance,

No, that won't work, because the object is dead by the time you get back to the caller as we saw above.  This needs to be done by the callee, with the caller assuming ownership, or you need to be _really_ super-careful in callees with these objects somehow.  I'd prefer the not-super-caref-required option.  ;)

There seems to not be any code using readPrincipals yet.  It would be good to have some, if only in test form...  That would also make the ownership bits easier to sort out.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> >+                           uint32_t aTag, uint32_t aAppId,
>
> aAppId is unused.  Why is it there?

First of all, this part of the patch is just moving existing code from
{Read,Write}FullySerializedObjects out into their own methods that are exposed
to spidermonkey. I'm very far from an expert in this area of the code base.

That said, the app id is used when serializing content principals.

> >+        if (!JS_ReadUint32Pair(aReader, &suffixLength, &specLength)) {
> >+            return nullptr;
>
> This function claims to return bool.

Good catch, sloppy refactoring on my part.

> >+    *aOutPrincipals = get(prin);
>
> What keeps the object alive when "prin" comes off the stack?  Seems like
> this should be an XPCOM-style outparam, where the callee addrefs it and the
> caller is responsible for releasing or something.

Yeah I had everything backwards, inversing the addrefing here as you suggest.

> >+    nsCOMPtr<nsIPrincipal> prin = PrincipalInfoToPrincipal(info, &rv);
>
> Why does that not need either a 'using' declaration or more namespacing?

There was a using at the top of the file, but I don't think it is carrying its
weight since the function is only used once. Going to just refer to this
function by its full namespacing.

> >+    return JS_WriteUint32Pair(aWriter, SCTAG_DOM_CONTENT_PRINCIPAL, 0) &&
>
> If it's always 0, why are we claiming it's an appid during deserialization?

Again, I was just moving existing code and I'm not that familiar with the
specifics. After rebasing on top of m-c, it is now used for content principals,
but not null or system principals.

> >+      static_assert(kSQLiteSchemaVersion == int32_t((22 << 4) + 0),
>
> This needs review from someone with idb knowledge, but seems fishy at first
> glance.  We're changing kSQLiteSchemaVersion but not providing any migration
> functionality.

Sure.

In fact, I think this doesn't actually need to happen until the next patch in my
queue which changes the structured cloning version number which forces this
version number to be changed too.

I'll see if I can trim this out.

> >+    MOZ_ASSERT(false, "not implemented");
>
> Any reason to not make that a MOZ_CRASH for the worker case?

Sounds good.

>
> > The caller is responsible for calling
> > + * JS_HoldPrincipals on the resulting JSPrincipals instance,
>
> No, that won't work, because the object is dead by the time you get back to
> the caller as we saw above.  This needs to be done by the callee, with the
> caller assuming ownership, or you need to be _really_ super-careful in
> callees with these objects somehow.  I'd prefer the not-super-caref-required
> option.  ;)

Yup.

> There seems to not be any code using readPrincipals yet.  It would be good
> to have some, if only in test form...  That would also make the ownership
> bits easier to sort out.

ReadKnownPrincipalType (nee FinishRead) is used by
StructuredCloneHolder::ReadFullySerializableObjects, but you are correct that
nsJSPrincipals::Read/JSReadPrincipalsOp is not used yet.

The next patch in my queue implements structured cloning for SavedFrames and
tests principals explicitly. If you don't feel strongly otherwise, I would
prefer to just wait for those tests. Since this patch is mostly just moving
existing code, and the moved code is still exercised by the old code paths, I
feel pretty secure in the existing structured cloning test coverage for now.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> That said, the app id is used when serializing content principals.

Ah, part of the mystery is resolved.  You patch is currently on top of the state from before bug 1167100 got backed out, looks like.  That patch removed the use of appid from this code, as far as I can tell.

> There was a using at the top of the file

That was for PrincipalToPrincipalInfo.  But ok, dropping the using and just using fully qualified names here is probably fine.

> I would prefer to just wait for those tests.

That sounds fine.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Allow embedders to tell SpiderMonkey how to structured clone principals

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=251e4b87deb9

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Allow embedders to tell SpiderMonkey how to structured clone principals

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This version should fix the override shadowing issues uncovered by the try push.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28fb62b87e5d

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Allow embedders to tell SpiderMonkey how to structured clone principals

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I have a fix for the (dumb on my part) unused already_AddRefed that destroyed that try run, but it seems bug 1167100 landed again, so I have to rebase on top of that again.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Allow embedders to tell SpiderMonkey how to structured clone principals

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Sorry for the bugspam! This new version is all rebased and uses unused_t to fix the failing assertions about not using the already_AddRefed (I would use .forget(aOutPtr) but the casting makes it grosser).

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=725b19e87ce3

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8669028 [details] [diff] [review]
Allow embedders to tell SpiderMonkey how to structured clone principals

>+    mozilla::ipc::PrincipalInfo info;

Drop the "mozilla::" since the whole file is "using namespace mozilla"?  Same in various other places here.

>+    unused << prin.forget();

Huh, apparently we have a specialization of operator<< for unused_t on already_AddRefed.  Neat.

That said, is there a reason we couldn't do:

  *aOutPrincipals = get(prin.forget().take());

?  Seems like that would be a slight smidgeon more explicit about where the ownership is going.  But if you think it's harder to read, what you have is fine as well.

>+nsJSPrincipals::write(JSContext* aCx, JSStructuredCloneWriter* aWriter)

This function somehow got weirdly indented during the revisions (6-space indent at toplevel inside the function, 2-space indent inside the if bodies).  Please fix the indentation in it to all be 4-space.

>Return false on
>+ * failure, true on success.

Two things I think merit pointing out in this comment:

1)  A false return doesn't mean an exception was thrown on cx, though one _might_ have been, right?

2)  What does this function promise in terms of initializing *outPrincipals on false return?  Seems like it needs to either promise to not modify the value or promise to set it to null; either one is OK, but it needs to be clear which one it is.  Right now the code does the "not modify" thing, I believe.

r=me

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Boris Zbarsky [:bz] from comment #16)
> >+    unused << prin.forget();
> 
> Huh, apparently we have a specialization of operator<< for unused_t on
> already_AddRefed.  Neat.
> 
> That said, is there a reason we couldn't do:
> 
>   *aOutPrincipals = get(prin.forget().take());
> 
> ?  Seems like that would be a slight smidgeon more explicit about where the
> ownership is going.  But if you think it's harder to read, what you have is
> fine as well.

Oh, cool! I didn't know about take(). I like this .take() version better.

> Two things I think merit pointing out in this comment:
> 
> 1)  A false return doesn't mean an exception was thrown on cx, though one
> _might_ have been, right?

I believe the only case where an exception isn't set on the cx is when we aren't on the main thread. The functions that take a JSStructuredCloneReader* set an exception on the JSStructuredCloneReader's cx.

I'm happy to put an exception on the cx in the not-main-thread case so we can give the guarantee that an exception will always exist on cx if we return false, but the existing code did not set an exception in this case and I'm not sure if that was just an oversight or omitted on purpose.

> 2)  What does this function promise in terms of initializing *outPrincipals
> on false return?  Seems like it needs to either promise to not modify the
> value or promise to set it to null; either one is OK, but it needs to be
> clear which one it is.  Right now the code does the "not modify" thing, I
> believe.

Yes, right now it does the "not modify" thing. I will document that.

Thanks for the reviews!

Comment 18

[Boris Zbarsky [:bzbarsky]]

> The functions that take a JSStructuredCloneReader* set an exception on the
> JSStructuredCloneReader's cx.

Ah, I had missed that.

OK, throwing an uncatchable exception in the non-mainthread case seems reasonable and doesn't need extra documentation.

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Allow embedders to tell SpiderMonkey how to structured clone principals

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Fixup last review comments.

Hopefully also fix the leaking nsJSPrincipals, I am pretty sure I found where that bug was (setting an already addref'd but not already_AddRefed (b/c of casts required between JSPrincipals* and nsIPrincipal*) to an nsCOMPtr), but only try will tell.

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

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8547896d582

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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