Closed Bug 1482720 Opened Last year Closed Last year

Crash in xul.dll!nsMsgMailNewsUrl::GetRef(nsTSubstring<char> & aRef)

Categories

(Thunderbird :: Mail Window Front End, defect, critical)

defect
Not set
critical

Tracking

(thunderbird_esr6062+ fixed, thunderbird61 wontfix, thunderbird62 wontfix, thunderbird63 fixed)

RESOLVED FIXED
Thunderbird 63.0
Tracking Status
thunderbird_esr60 62+ fixed
thunderbird61 --- wontfix
thunderbird62 --- wontfix
thunderbird63 --- fixed

People

(Reporter: infofrommozilla, Assigned: jorgk)

References

Details

(Keywords: crash, regression)

Attachments

(2 files, 4 obsolete files)

Attached file stacktrace.txt
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20180810195043

Steps to reproduce:

Look at an email with the content: "smtp<colon><colon>foo"
( <colon><colon> -> :: )



Actual results:

TB crashes when the email is shown.
In the newsgroup mozilla.support.bugzilla there is a thread with the subject: "Gmail Sendmail Parameter Broken". The first post contains such a text.

The regression window:
last good: Comm-Central:c7f944624807 / Mozilla-Central:2c4a055ed5d4
first bad: Comm-Central:e2d00a308beb / Mozilla-Central:7b55d395bb63
Thanks for filing the bug.

Confirmed with latest nightly  bp-04de08d0-3ec9-48ed-a51d-6bd260180812 xul.dll@0x45024 | xul.dll@0x115209d | xul.dll@0x39898c8 | xul.dll@0x3844809 | xul.dll@0x19673bb | xul.dll@0x4319681 | xul.dll@0x3827ff9 | xul.dll@0x496685f | xul.dll@0x1ab1e00 | xul.dll@0x1ab2033 | xul.dll@0x390f9d1 | xul.dll@0x39108fa | xul.dll@0x1ab1...
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, regression
(In reply to Alfred Peters III from comment #0)
> Created attachment 8999442 [details]
> stacktrace.txt

>	xul.dll!nsCOMPtr<nsIURL>::operator->() Line 808	C++
 	xul.dll!nsMsgMailNewsUrl::GetRef(nsTSubstring<char> & aRef) Line 728	C++
 	xul.dll!mozilla::dom::Link::GetHash(nsTSubstring<char16_t> & _hash) Line 777	C++
 	xul.dll!mozilla::dom::HTMLAnchorElement_Binding::get_hash(JSContext * cx, JS::Handle<JSObject *> obj, mozilla::dom::HTMLAnchorElement * self, JSJitGetterCallArgs args) Line 1797	C++
 	xul.dll!mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions>(JSContext * cx, unsigned int argc, JS::Value * vp) Line 3187	C++
 	xul.dll!CallJSNative(JSContext * cx, bool(*)(JSContext *, unsigned int, JS::Value *) native, const JS::CallArgs & args) Line 445	C++
 	xul.dll!js::InternalCallOrConstruct(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct) Line 533	C++
 	xul.dll!InternalCall(JSContext * cx, const js::AnyInvokeArgs & args) Line 585	C++
(In reply to Alfred Peters III from comment #1)
> The regression window:
> last good: Comm-Central:c7f944624807 / Mozilla-Central:2c4a055ed5d4
> first bad: Comm-Central:e2d00a308beb / Mozilla-Central:7b55d395bb63
M-C: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2c4a055ed5d4&tochange=7b55d395bb63
C-C: https://hg.mozilla.org/comm-central/pushloghtml?fromchange=c7f944624807&tochange=e2d00a308beb

Looks like bug 1447272. Alice, could you please narrow this down a bit more for us.
Flags: needinfo?(alice0775)
I've looked at this in the debugger:
Link::GetHash() calls |nsresult rv = uri->GetRef(ref);| on an nsSmtpUrl, and that in turn calls |m_baseURL->GetRef(aRef);| in nsMsgMailNewsUrl::GetRef(). Sadly m_baseURL is null, so we crash. That shouldn't happen, all nsMsgMailNewsUrls should have m_baseURL populated. Something went wrong during the creation.

Fortunately the error got introduced after the branch to TB 60, so that's not affected.

It would be good to have a closer regression window to pin it down faster.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Also crashes when the message contains mailbox::foo.

Looks like this is caused by https://hg.mozilla.org/comm-central/rev/481987fc6713 where we removed creating the m_baseURL URLs in the CTORs.
Flags: needinfo?(alice0775)
Aha(In reply to Jorg K (GMT+2) from comment #6)
> Also crashes when the message contains ....

So that's why the bugmail from comment 6 keeps crashing when trying to open!
I tried with addbook: and mailto: URLs also affected by that patch and saw no crash. So I'll just add something back into the CTOR of the nsMsgMailNewsUrls.
Here's the fix for the bug as it was reported. Now I need to fix mailbox URLs as well so Magnus can ready comment #6 ;-)

The issue is that the code removed from the constructors cannot be restored since M-C removed those calls.

The key is to call SetSpecInternal() successfully so that a URL is allocated. I need to see where that happens for mailboxes.
https://dxr.mozilla.org/comm-central/search?q=regexp%3Avoid.*SetSpecInternal&redirect=false
finds four cases where we ignore the status of SetSpecInternal(): In CreateStartupUrl() which will hopefully be removed soon in bug 110689, nsMailboxService::NewURI() which causes the problem here, and MaildirStoreParser::TimerCallback().

Sadly the comment in nsMailboxService::NewURI() says:
  // SetSpec calls below may fail if the mailbox url is of the form
  // mailbox://<account>/<mailbox name>?... instead of
  // mailbox://<path to folder>?.... This is the case for pop3 download urls.
  // We know this, and the failure is harmless.
Well, failure is no longer harmless since we will have a URL without an inner base URL.
Thanks Alice, I suspected rev. 481987fc6713.
Attached patch 1482720-fix-URL-crash.patch (v2) (obsolete) — Splinter Review
This is the right think to do. Most likely it will break the following functions:
- download headers only (pop)
- download only messages smaller than ... (pop)
Those show mailbox URLs of a different kind in the message pane. Or maybe those crash already, I haven't tried.

The fix here in necessary, there is no way around it.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=92100b660f9e63750372b2c88bd9d6f79d2eccff

I'll do a follow-up to fix those, but for now we can land the crash fix so people don't crash any more; mind you, the crash has been there since March and no one has complained so far.
Attachment #8999457 - Attachment is obsolete: true
Attachment #8999460 - Flags: review?(mkmelin+mozilla)
Attachment #8999460 - Flags: review?(acelists)
Yep, "Download the rest of the message." is now broken since it has a funny mailbox URL which is now failing. Sigh.
Comment on attachment 8999460 [details] [diff] [review]
1482720-fix-URL-crash.patch (v2)

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

How does the error checking actually fix the problem? I thought you will add some initializing of the missing members.
Ah, you say this just prevents the crash, but will not make the links work?
No, the initialising of the base URL happens in SetSpecInternal(). If that call fails, we're hosed.

I'm onto fixing the links, give me 10 minutes more.
Attached patch 1482720-fix-URL-crash.patch (v3) (obsolete) — Splinter Review
OK, here is the full solution also cleaning up some technical debt of having half-baked URLs in the system.

URLs of the form mailbox://user@domain@server/folder?number=nn *are* valid URLs, only the silly nsMailboxUrl::ParseUrl() can't handle them. That's why nsMailboxUrl::SetSpecInternal() fails but not nsMsgMailNewsUrl::SetSpecInternal().

We can have the best of both worlds by not attempting to parse those.

Note that the Find("///") check is copied from nsMailboxUrl::GetNormalizedSpec(). Kent James approved, so it must be good ;-)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cc3abcd158274e355025a1984a8bb137d5da5e44
Attachment #8999460 - Attachment is obsolete: true
Attachment #8999460 - Flags: review?(mkmelin+mozilla)
Attachment #8999460 - Flags: review?(acelists)
Attachment #8999473 - Flags: review?(acelists)
Attached patch 1482720-fix-URL-crash.patch (v4) (obsolete) — Splinter Review
Another call site ignoring the alleged error. Guess what, there is no error, I added a printf() to check it. This code runs for "edit as new" and forward.

URL is in the form:
mailbox-message://nobody@Local%20Folders/XXX#10?fetchCompleteMessage=true&editasnew=true

So if that had failed, we would have seen crashes already.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5109046890852da2da8999b7d106cc1071736ff6
Attachment #8999473 - Attachment is obsolete: true
Attachment #8999473 - Flags: review?(acelists)
Attachment #8999477 - Flags: review?(acelists)
Had to kill an incorrect comment.
Attachment #8999477 - Attachment is obsolete: true
Attachment #8999477 - Flags: review?(acelists)
Attachment #8999478 - Flags: review?(acelists)
Comment on attachment 8999478 [details] [diff] [review]
1482720-fix-URL-crash.patch (v4b)

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

Thanks, this works for me, fixes the crash on smtp::foo link in message and the "link" isn't underlined (clickable).
A mailbox://something link is clickable but does nothing.
A mailbox:// link to "load the rest of the message" works.

But displaying the message with smtp://foo produces an error in the console:
NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsISmtpUrl.init] nsSMTPProtocolHandler.js:27

Do we want this for just underlining a potential link in message? Or can we somehow silence this?
Attachment #8999478 - Flags: review?(acelists) → review+
I think we should add a test to catch this (the crash). Might as well just add the crasher string into a message we load somewhere, and maybe an explanation too.

As for the partial POP messages and headers only, I might well consider dropping support for that. The data amount of a few mails, even if junk, is quite small in today's world. A single load of facebook will download many megabytes already (and people do that). I'm not sure why anyone would opt to shave of a few 100k per day given the inconvenience it causes to use the feature.
(In reply to :aceman from comment #20)
> Do we want this for just underlining a potential link in message? Or can we
> somehow silence this?
In cases where the URL can't be created due to an invalid spec, the text in the message is no longer linkified, that's expected. Strangely the SMPT protocol handler is implemented in JS in nsSMTPProtocolHandler.js, so it makes some noise in the console if newURI() fails. All I could do is:
try {
  url.init(aSpec);
catch (e) {
  throw(e);
}
but Magnus doesn't like on-throwing the error (bug 1424359 comment #26: "Catching just to throw it makes no sense").

(In reply to Magnus Melin from comment #21)
> I think we should add a test to catch this (the crash). Might as well just
> add the crasher string into a message we load somewhere, and maybe an
> explanation too.
I have a few test cases for the various protocols:
smtp::foo, addbook:foo, mailbox:foo, imap:foo, mailto:foo, news:foo, ldap:foo which show varying results of interpreting the link. Yes, we can add a test somewhere.

> As for the partial POP messages and headers only, I might well consider
> dropping support for that.
Hmm, once again, you forgot the conservative user base. Every time the "download rest of message" feature broke for "download headers only" and "download only messages smaller than", we got lots of complaints.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/bbfc8e24eae2
always check status of SetSpecInternal() so bad specs don't cause a crash. r=aceman
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 63.0
Comment on attachment 8999478 [details] [diff] [review]
1482720-fix-URL-crash.patch (v4b)

We could consider this for uplift so we don't run our major functionality based on ignoring statuses :-(
Attachment #8999478 - Flags: approval-comm-esr60?
(In reply to Jorg K (GMT+2) from comment #22)
> > As for the partial POP messages and headers only, I might well consider
> > dropping support for that.
> Hmm, once again, you forgot the conservative user base. Every time the
> "download rest of message" feature broke for "download headers only" and
> "download only messages smaller than", we got lots of complaints.

A conservative user base doesn't mean we can't ever change anything.

Of course there would be some users using these features since they are in the UI - that's why we shouldn't put or keep nonsense in the UI (though this has a use case back in early 2000s). What's less clear is do they have a good reason to use it except "because I can". I have more concern for the 99% (?) than for the 1% who happened to select it.

If some feature in the UI will make your user experience abysmal or make you look bad, we should't be exposing it there.

Instead of breaking them we would offer the normal experience. Breakage always gives complaints.
Let's discuss this in another bug. Once again we sorely notice information about what our users are using (telemetry). To some this might be an interesting feature since users might download headers only and then decide whether they want to see the entire message or discard it based on the header. We also have users on satellite internet (I can provide bug numbers) where every downloaded byte costs. Surely they fall into the 1% category, but 1% are still 250.000 users who I'd prefer not to upset.
Messed up grammar:
Once again we sorely notice *missing* information about what our users are using (telemetry).
Catching the exception just to throw it makes no sense for sure. I meant if the nsSMTPProtocolHandler.js is initing the url just for the purposes of underlining it in the message body, then failure to do so could be silently catched and not reported.

I'm also for keeping the feature of downloading headers or partial messages only as long as we can keep it afloat.
It is useful for slow or metered connections, which still exist even in Europe (slow mobile data plans or limited or credit-based data plans). Not to mention e.g. Africa.
I myself am using this feature, albeit the need has lowered in urgency recently.
Blocks: 1447272
(In reply to :aceman from comment #20)
> smtp://foo

(In reply to Jorg K (GMT+2) from comment #22)
> smtp::foo, addbook:foo, mailbox:foo, imap:foo, mailto:foo, news:foo,
> ldap:foo

In Plain Text view smtp://foo, addbook:foo, mailbox:foo, mailto:foo, news:foo and ldap:foo are turned into a link -
smtp::foo and imap:foo not.

Everything ok so far. But if I click on smtp://foo or ldap:foo (only of these) TB still crashes.

Plus: If the email is shown again, TB crashes on view.
Clearing the browsing history fixes the second issue.
Blocks: 1483043
Indeed, smtp://foo and ldap:foo get linkified and crash when clicked, not only in a plaintext view.

Crash is in SerializeURI() on MOZ_CRASH("All IPDL URIs must be serializable!").

I filed bug 1483043 for that.
Attachment #8999478 - Flags: approval-comm-esr60? → approval-comm-esr60+
You need to log in before you can comment on or make changes to this bug.