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

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

unspecified
mozilla39
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(e10sm5+, firefox39 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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).
Posted patch patch v1 (obsolete) — Splinter Review
Note, too, that this matches current behavior as null principals don't have an origin (and are not file URIs, which are treated specially).
Attachment #8509833 - Flags: review?(bugs)
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.
Attachment #8509833 - Flags: review?(bugs) → review-
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?
Posted patch Add a testSplinter Review
Attachment #8509833 - Attachment is obsolete: true
Attachment #8564410 - Flags: review?(bugs)
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.
Attachment #8564412 - Flags: review?(bzbarsky)
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).
Attachment #8564413 - Flags: review?(mcmanus)
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 :) )
Attachment #8564410 - Flags: review?(bugs) → review-
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.
Attachment #8564412 - Flags: review?(bzbarsky) → review+
Attachment #8564413 - Flags: review?(mcmanus) → review+
(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 on attachment 8564410 [details] [diff] [review]
Add a test

Please see comment 9.
Attachment #8564410 - Flags: review- → review?(bugs)
Attachment #8564410 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/fa5a5dbde1c7
https://hg.mozilla.org/mozilla-central/rev/9f4da60dbfff
https://hg.mozilla.org/mozilla-central/rev/bc2be5c8ab2a
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.