Closed Bug 1087646 Opened 10 years ago Closed 10 years ago

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


(Core :: Networking: Cookies, defect)

Not set



Tracking Status
e10s m5+ ---
firefox39 --- fixed


(Reporter: mrbkap, Assigned: mrbkap)



(3 files, 1 obsolete file)

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).
Attached 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

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?
Attached 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+
Closed: 10 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.