Closed Bug 1117337 Opened 10 years ago Closed 9 years ago

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


(Toolkit :: Places, defect)

Not set



Tracking Status
e10s m5+ ---
firefox38 --- fixed


(Reporter: martijn.martijn, Assigned: mrbkap)



(Keywords: crash, testcase, Whiteboard: [e10s])

Crash Data


(4 files, 5 obsolete files)

Attached file 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
This only happens in e10s, btw.
Blocks: core-e10s
Summary: crash in mozilla::ipc::SerializeURI(nsIURI*, mozilla::ipc::URIParams&) → [e10s] Crash in mozilla::ipc::SerializeURI(nsIURI*, mozilla::ipc::URIParams&)
Whiteboard: [e10s]
tracking-e10s: --- → ?
OS: Mac OS X → All
Assignee: nobody → mrbkap
Attached patch patch v1 (obsolete) — Splinter Review

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.
Attachment #8560758 - Flags: review?(bent.mozilla)
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).
Attachment #8560758 - Attachment is obsolete: true
Attachment #8560758 - Flags: review?(bent.mozilla)
Attached patch Rip out unused code (obsolete) — Splinter Review
This patch doesn't actually change any behavior. It simply rips out some code that we want to avoid from now-on.
Attachment #8561688 - Flags: review?(bent.mozilla)
This adds correct serialization for moz-icon URIs.
Attachment #8561689 - Flags: review?(bent.mozilla)
I'm tired of this bug crashing my e10s windows!
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
Attachment #8561688 - Flags: review?(bent.mozilla) → review+
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.
Attachment #8561689 - Flags: review?(bent.mozilla) → review+
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.
Attachment #8563153 - Flags: review?(seth)
Attached patch Construct via opaque CID (obsolete) — Splinter Review
For completeness...
Flags: needinfo?(mrbkap)
Attachment #8563154 - Flags: review?(bent.mozilla)
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?
Attachment #8563154 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8563153 [details] [diff] [review]
Make icon URIs constructable via XPCOM.

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

Looks good!
Attachment #8563153 - Flags: review?(seth) → review+
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.
Attachment #8561688 - Attachment is obsolete: true
Attachment #8561689 - Attachment is obsolete: true
Attachment #8563153 - Attachment is obsolete: true
Attachment #8563154 - Attachment is obsolete: true
Attachment #8563543 - Flags: review+
QA Whiteboard: [good first verify][verify in Nightly only]
You need to log in before you can comment on or make changes to this bug.