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

RESOLVED FIXED in Thunderbird 66.0

Status

defect
--
critical
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: emilio, Assigned: jorgk)

Tracking

({crash, regression, topcrash-thunderbird})

unspecified
Thunderbird 66.0

Thunderbird Tracking Flags

(thunderbird65 fixed, thunderbird66 fixed)

Details

(crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

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://emilio%40crisal%2Eio@emiliocobos.net: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.
[@ mozilla::ipc::SerializeURI ] maps to similar issue previously fixed bug 1483043
Severity: normal → critical
Crash Signature: [@ mozilla::ipc::SerializeURI ]
Component: Untriaged → General
Keywords: crash
Assignee

Comment 2

5 months ago
Please attach it, saves me constructing my own.
Reporter

Comment 3

5 months ago
Posted file 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).
Assignee

Updated

5 months ago
Attachment #9032427 - Attachment mime type: message/rfc822 → text/plain
Assignee

Comment 4

5 months ago
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.
Flags: needinfo?(alice0775)
Keywords: regression
Reporter

Comment 5

5 months ago
(Or bug 1487249, which landed the code that's crashing)
Assignee

Comment 6

5 months ago
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

5 months ago
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
Flags: needinfo?(alice0775)
Assignee

Comment 8

5 months ago
Thanks, don't worry, it's around the bug I mentioned.
Assignee

Comment 9

5 months ago
Ben and Neil, can you check this for JsAccount. That will need some work, too, since it has it's own URL class.
Flags: needinfo?(neil)
Flags: needinfo?(ben.bucksch)
Summary: Reproducible crash in daily when clicking on an imap email link with href="#foo" → Crash in daily when clicking on an email link (mailbox, IMAP, NNTP) with reference like href="#foo"
Assignee

Comment 11

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

Totally wrong, need to implement nsIIPCSerializableURI.
Attachment #9032503 - Attachment is obsolete: true
Assignee

Comment 12

5 months ago
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?
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Flags: needinfo?(neil)
Flags: needinfo?(emilio)
Flags: needinfo?(ben.bucksch)
Attachment #9032523 - Flags: review?(nika)
Attachment #9032523 - Flags: review?(mkmelin+mozilla)
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.
Assignee

Comment 14

5 months ago
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.
Attachment #9032523 - Flags: review?(honzab.moz)
Reporter

Comment 15

5 months ago
Yeah, I'd try a Necko person, not sure who other person would review that.
Flags: needinfo?(emilio)
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
Attachment #9032523 - Flags: review?(nika)
Assignee

Comment 17

5 months ago
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 on attachment 9032523 [details] [diff] [review]
1515337-make-serialisable2.patch

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

rs=mkmelin but do add the comment
Attachment #9032523 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 19

5 months ago
Attachment #9032523 - Attachment is obsolete: true
Attachment #9032523 - Flags: review?(honzab.moz)
Attachment #9032633 - Flags: review+

Comment 20

5 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b382906f387f
Bug 1512356 follow-up: Make nsMsgMailNewsUrl implement nsIIPCSerializableURI. rs=mkmelin DONTBUILD
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Assignee

Updated

5 months ago
Target Milestone: --- → Thunderbird 66.0
Assignee

Updated

5 months ago
Attachment #9032633 - Flags: approval-comm-beta+
See Also: → 1516636
You need to log in before you can comment on or make changes to this bug.