1087646 - e10s crash when setting cookies in a document that has null principals

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

With e10s enabled:

== STR ==

type data:text/html,<script>document.cookie="a=b";alert(document.cookie)</script> into the URL bar.

== Expected result ==

A blank alert.

== Actual result ==

The content process crashes with the following message:

Hit MOZ_CRASH(All IPDL URIs must be serializable or an allowed scheme!) at /home/mrbkap/work/tree2/mozilla/ipc/glue/URIUtils.cpp:83

because the data: document has a null principal and we can't serialize them across IPC boundaries (which is a weird concept that I don't think we should allow).

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

Note, too, that this matches current behavior as null principals don't have an origin (and are not file URIs, which are treated specially).

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8509833 [details] [diff] [review]
patch v1

This looks very hacky.
Why we add special cases only in those 3 places?
I assume we crash because of http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#2011

We really need to be able to support null principals properly in content process too.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Also, the user of cookie service really shouldn't be forced to check what kind of URI is passed to it.
Maybe one way to fix this is to return early in the child-side of the e10s-cookie service?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Add a test

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Properly serialize nullprincipal URIs across IPC

After talking to smaug (a while back) and bent, I decided to allow serializing null principal URIs with the same rule as we serialize null principals: you can do it, but it doesn't round trip. This fixes this bug alone.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: fast-path null principal'd documents

This is an optimization that avoids the IPC overhead if we're going to send a null principal URI over (we know that it won't have a cookie).

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8564410 [details] [diff] [review]
Add a test

You have processResults defined, but nothing calls it.
Is something missing from the patch or what?

(never seen ... syntax before :) )

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8564412 [details] [diff] [review]
Properly serialize nullprincipal URIs across IPC

Not sure why GetScriptLocation got moved around there...

r=me apart from that.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Olli Pettay [:smaug] from comment #7)
> You have processResults defined, but nothing calls it.
> Is something missing from the patch or what?

Sorry, I copy/pasted the test from another test that was much more involved. processResults was left over from that test. This one is mostly a crashtest: if we get through it without crashing, we don't have any results to process. I've removed the offending function locally.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8564410 [details] [diff] [review]
Add a test

Please see comment 9.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

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

I need to attach a new version of attachment 8564410 [details] [diff] [review] before check-in.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa5a5dbde1c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f4da60dbfff
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc2be5c8ab2a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa5a5dbde1c7
https://hg.mozilla.org/mozilla-central/rev/9f4da60dbfff
https://hg.mozilla.org/mozilla-central/rev/bc2be5c8ab2a