1117337 - [e10s] Crash in mozilla::ipc::SerializeURI(nsIURI*, mozilla::ipc::URIParams&)

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Martijn Wargers (dead)]

Attachment: ishtmllink.htm

See url or testcase to see the crash in current trunk build.

This bug was filed from the Socorro interface and is 
report bp-df95a9cb-7030-4b36-8ec3-8453e2150103.
=============================================================
0 	XUL 	mozilla::ipc::SerializeURI(nsIURI*, mozilla::ipc::URIParams&) 	ipc/glue/URIUtils.cpp
1 	XUL 	mozilla::places::History::VisitURI(nsIURI*, nsIURI*, unsigned int) 	toolkit/components/places/History.cpp
2 	XUL 	nsDocShell::AddURIVisit(nsIURI*, nsIURI*, nsIURI*, unsigned int, unsigned int) 	docshell/base/nsDocShell.cpp
3 	XUL 	nsDocShell::OnNewURI(nsIURI*, nsIChannel*, nsISupports*, unsigned int, bool, bool, bool) 	docshell/base/nsDocShell.cpp
4 	XUL 	nsDocShell::OnLoadingSite(nsIChannel*, bool, bool) 	docshell/base/nsDocShell.cpp
5 	XUL 	nsDocShell::CreateContentViewer(char const*, nsIRequest*, nsIStreamListener**) 	docshell/base/nsDocShell.cpp
6 	XUL 	nsDSURIContentListener::DoContent(char const*, bool, nsIRequest*, nsIStreamListener**, bool*) 	docshell/base/nsDSURIContentListener.cpp
7 	XUL 	nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) 	uriloader/base/nsURILoader.cpp

Comment 1

[Martijn Wargers (dead)]

This only happens in e10s, btw.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

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

First note: javascript and about URIs both inherit from nsSimpleURI, which, in turn, inherits from nsIIPCSerializableURI. That means that we can't get into the generic URI case for either of those.

This crash was first noticed in bug 789059 (with a followup bug 801882, that I'll dupe here) where we skirted around the issue by simply avoiding sending a message with a serialized moz-icon URI at all.

I can't see a reason to not serialize moz-icon URIs. We could hack around this bug by avoiding sending the "visited?" message in the places code, but then we'd still crash if the user explicitly navigated to a moz-icon URI. With this patch, both cases should work (and I don't think there are any security concerns to be had either)..

I actually wasted some time making nsMozIconURI inherit nsIIPCSerializableURI before realizing that we actually do a good job sticking all of our state in iconURI.spec, meaning that simply passing it and parsing it on the other end creates the proper object in the right state.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

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

After talking with bent, we decided to opt for real serialization for moz-icon URIs (which means that I can rip out the generic schemes now).

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Rip out unused code

This patch doesn't actually change any behavior. It simply rips out some code that we want to avoid from now-on.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Properly serialize moz-icon URIs.

This adds correct serialization for moz-icon URIs.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm tired of this bug crashing my e10s windows!

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8561688 [details] [diff] [review]
Rip out unused code

Review of attachment 8561688 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/URIUtils.cpp
@@ +42,2 @@
>      MOZ_CRASH("All IPDL URIs must be serializable or an allowed "
>                "scheme!");

Nit: There's no allowed scheme now

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8561689 [details] [diff] [review]
Properly serialize moz-icon URIs.

Review of attachment 8561689 [details] [diff] [review]:
-----------------------------------------------------------------

Fingers... crossed...

::: image/decoders/icon/nsIconURI.cpp
@@ +594,5 @@
> +  IconURIParams params;
> +
> +  if (mIconURL) {
> +    URIParams iconURLParams;
> +    SerializeURI(mIconURL, iconURLParams);

Bail out here if iconURLParams wasn't set properly

@@ +613,5 @@
> +bool
> +nsMozIconURI::Deserialize(const URIParams& aParams)
> +{
> +  if (aParams.type() != URIParams::TIconURIParams) {
> +    NS_ERROR("Received unknown URI from other process!");

I'd MOZ_ASSERT(false) here.

@@ +622,5 @@
> +  if (params.uri().type() != OptionalURIParams::Tvoid_t) {
> +    nsCOMPtr<nsIURI> uri = DeserializeURI(params.uri().get_URIParams());
> +    mIconURL = do_QueryInterface(uri);
> +    if (!mIconURL) {
> +      NS_ERROR("bad nsIURI passed");

And here.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4817b6ad61
https://hg.mozilla.org/integration/mozilla-inbound/rev/016b3f06add1

Comment 11

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

Backed out for b2g build bustage in https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d0685f13f2

https://treeherder.mozilla.org/logviewer.html#?job_id=6543177&repo=mozilla-inbound

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make icon URIs constructable via XPCOM.

In order to fix the b2g bustage, I'm going to use XPCOM to create nsMozIconURI instances. To do so, I'm exposing the CID in a location that is unconditionally included in the build, and then it'll only be used if we actually can create them.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Construct via opaque CID

For completeness...

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8563154 [details] [diff] [review]
Construct via opaque CID

I guess you can also undo that extra EXPORT you added in the previous patch then?

Comment 15

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8563153 [details] [diff] [review]
Make icon URIs constructable via XPCOM.

Review of attachment 8563153 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make icon URIs constructable via XPCOM.

These are the final versions of the patches. I can't push to try right now because everything is closed. Hopefully things will open and this bug will be fixed sooner rather than later.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Get rid of this generic path in favor of explicit (de)serialization of URIs.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Properly serialize moz-icon URIs.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Let's try this again.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0792fde211c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c38815d00a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3d38ae88df7

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0792fde211c6
https://hg.mozilla.org/mozilla-central/rev/0c38815d00a7
https://hg.mozilla.org/mozilla-central/rev/c3d38ae88df7