Closed
Bug 1117337
Opened 10 years ago
Closed 9 years ago
[e10s] Crash in mozilla::ipc::SerializeURI(nsIURI*, mozilla::ipc::URIParams&)
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: martijn.martijn, Assigned: mrbkap)
References
Details
(Keywords: crash, testcase, Whiteboard: [e10s])
Crash Data
Attachments
(4 files, 5 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•9 years ago
|
||
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]
Updated•9 years ago
|
tracking-e10s:
--- → ?
Updated•9 years ago
|
OS: Mac OS X → All
Updated•9 years ago
|
Assignee: nobody → mrbkap
Assignee | ||
Comment 2•9 years ago
|
||
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.
Attachment #8560758 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
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+
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4817b6ad61 https://hg.mozilla.org/integration/mozilla-inbound/rev/016b3f06add1
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
Flags: needinfo?(mrbkap)
Assignee | ||
Comment 12•9 years ago
|
||
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)
Assignee | ||
Comment 13•9 years ago
|
||
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 15•9 years ago
|
||
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+
Assignee | ||
Comment 16•9 years ago
|
||
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+
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8563544 -
Flags: review+
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8563545 -
Flags: review+
Assignee | ||
Comment 19•9 years ago
|
||
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•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0792fde211c6 https://hg.mozilla.org/mozilla-central/rev/0c38815d00a7 https://hg.mozilla.org/mozilla-central/rev/c3d38ae88df7
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Updated•9 years ago
|
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.
Description
•