1515337 - Crash in daily when clicking on an email link (mailbox, IMAP, NNTP) with reference like href="#foo"

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Emilio Cobos Álvarez [:emilio]]

See https://crash-stats.mozilla.com/report/index/5f2eba54-610d-48eb-89cb-222220181219

The URL is from an imap message, and it's a local ref, I guess:

  imap://[e-mail]:143/fetch%3EUID%3E.INBOX.webkit.webkit-changes%3E14167#trunkSourceWebCorepageFrameViewcpp

In the raw source of the email I see:

  <li><a href="#trunkSourceWebCorepageFrameViewcpp">trunk/Source/WebCore/page/FrameView.cpp</a></li>

I can attach the whole thing if you want, or forward it, or something, this is a public email.

In the browser I get a "this address wasn't understood" message, as expected, I guess.

I'm not sure if it's a recent regression or not.

Comment 1

[Wayne Mery (:wsmwk)]

[@ mozilla::ipc::SerializeURI ] maps to similar issue previously fixed bug 1483043

Comment 2

[Jorg K (CEST = GMT+2)]

Please attach it, saves me constructing my own.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Attachment: crash.eml

That's it. Any of the webkit-changes@ mails repros for me, just click on a path (trunk/CMakeLists.txt for example).

Note that the fact that I'm using imap:// probably affects the end result, since in the email the links are just local (href="#trunkCMakeListstxt", for example).

Comment 4

[Jorg K (CEST = GMT+2)]

Importing the message into IMAP or local folder and then clicking onto
  trunk/CMakeLists.txt
  trunk/ChangeLog
works fine in TB 60 ESR and TB 64 beta. The message is scrolled so these anchors come into view.

Trunk crashes with:
xul.dll!mozilla::ipc::SerializeURI(nsIURI * aURI, mozilla::ipc::URIParams & aParams) Line 47	C++
xul.dll!mozilla::ipc::SerializeURI(nsIURI * aURI, mozilla::ipc::OptionalURIParams & aParams) Line 62	C++
xul.dll!mozilla::ipc::WriteIPDLParam<nsIURI *&>(...) Line 61	C++
xul.dll!mozilla::dom::PWindowGlobalChild::SendUpdateDocumentURI(nsIURI * aUri) Line 42	C++
xul.dll!nsIDocument::SetDocumentURI(nsIURI * aURI) Line 2909	C++
xul.dll!nsDocShell::InternalLoad(...) Line 9304	C++
xul.dll!nsDocShell::OnLinkClickSync(...) Line 12774	C++

Most likely a fallout from the latest principal/URI serialisation changes in bug 1512356 which was an adaption to bug 1487249 and friends.

Alice, can you please confirm this or find the correct regression.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

(Or bug 1487249, which landed the code that's crashing)

Comment 6

[Jorg K (CEST = GMT+2)]

Umm, that's what I said. And as far as I can see, the the crash is this: URIUtils.cpp:47

  nsCOMPtr<nsIIPCSerializableURI> serializable = do_QueryInterface(aURI);
  if (!serializable) {
47  MOZ_CRASH("All IPDL URIs must be serializable!");
  }

That's not new code, but the requirement for those URLs to be serialisable is new. I'd have to check what sort of URL causes the crash. Sadly, the MSVS debugger after compiling with clang-cl isn't very good and only says nsISupports :-(

Comment 7

[Alice0775 White]

Regression window:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&searchStr=windows&tochange=e993eb9d7c1b74cf3506fe74be8b44fa5dfac86e&fromchang=f51f341a4d9cb315ba830d242e246442458bfb1c

I cannot be bisection, because Tb crashes when clicking the message due to Bug 1512356

Comment 8

[Jorg K (CEST = GMT+2)]

Thanks, don't worry, it's around the bug I mentioned.

Comment 9

[Jorg K (CEST = GMT+2)]

Ben and Neil, can you check this for JsAccount. That will need some work, too, since it has it's own URL class.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1515337-make-serialisable.patch _WIP for mailbox, NOT working

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 9032503 [details] [diff] [review]
1515337-make-serialisable.patch _WIP for mailbox, NOT working

Totally wrong, need to implement nsIIPCSerializableURI.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1515337-make-serialisable2.patch

This fixes the crash and restores the original behaviour.

Who can review this? Do we need to implement Deserialize()?

Clearing NI for Neil and Ben since JsAccount is not affected, it's handled in the superclass.

Emilio, it's your bug can you find a reviewer? Maybe Nika?

Comment 13

[Andrew McCreight [:mccr8]]

Looks like Deserialize was removed in bug 1441688. mayhemer reviewed that, so maybe they could review this, too. URI stuff is kind of network-y.

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 9032523 [details] [diff] [review]
1515337-make-serialisable2.patch

Hmm, I'm a little confused, since I copied my stuff from here:
https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/netwerk/base/nsStandardURL.cpp#3345
https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/netwerk/base/nsStandardURL.cpp#3372
Well, looks like they have their own private deserialise. OK, I'll try Honza since Valentin is on PTO.

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Yeah, I'd try a Necko person, not sure who other person would review that.

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9032523 [details] [diff] [review]
1515337-make-serialisable2.patch

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

Unfortunately this patch doesn't actually make nsMsgMailNewUrl serializable, but it probably functions given that thunderbird is unlikely to start using IPC much anytime soon. This does mean that sends of URIs which go within-process will be inaccurate, which I can see causing issues down the line.

This is probably fine as a hack with a nice comment noting that it's probably going to cause things to horribly break in the future, but I don't know if it's a good final solution :-/

::: mailnews/base/util/nsMsgMailNewsUrl.cpp
@@ +104,5 @@
>  }
>  
> +void nsMsgMailNewsUrl::Serialize(mozilla::ipc::URIParams &aParams) {
> +  nsCOMPtr<nsIIPCSerializableURI> serializable = do_QueryInterface(m_baseURL);
> +  return serializable->Serialize(aParams);

It's a touch unfortunate that sending this URI over IPC would cause the incorrect URI to be deserialized on the other side, but I don't see any good way of handling that without adding more cases to URIParmas. This is also likely the reason why Deserialize isn't causing linker errors, as it is only needed if the URI can actually be deserialized, which this URI is not.

@@ +105,5 @@
>  
> +void nsMsgMailNewsUrl::Serialize(mozilla::ipc::URIParams &aParams) {
> +  nsCOMPtr<nsIIPCSerializableURI> serializable = do_QueryInterface(m_baseURL);
> +  return serializable->Serialize(aParams);
> +}  

note: trailing whitespace

Comment 17

[Jorg K (CEST = GMT+2)]

Thanks for the comments (I spotted the white-space, but I didn't want to cause more noise). If you look at the Write() and Read() methods, there we also have a hack you suggested yourself.

I'm not sure where your comments leave me, we need a short-term fix for this crash, even if it's a hack. We already have bug 1512698 on file which will revisit the area.

> Unfortunately this patch doesn't actually make nsMsgMailNewUrl serializable ...
Right, I changed the commit message.

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9032523 [details] [diff] [review]
1515337-make-serialisable2.patch

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

rs=mkmelin but do add the comment

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1515337-make-serialisable2.patch - with comments

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b382906f387f
Bug 1512356 follow-up: Make nsMsgMailNewsUrl implement nsIIPCSerializableURI. rs=mkmelin DONTBUILD

Comment 21

[Jorg K (CEST = GMT+2)]

Beta (TB 65):
https://hg.mozilla.org/releases/comm-beta/rev/83a1ba1a1c350ead0d7b83e5244c5ca061b04d91