1642935 - thunderbird gtest permafail: gtest | application crashed [@ mozilla::net::CookieCommons::URIToSchemeType(nsIURI*)]

Status: RESOLVED FIXED

(MailNews Core :: Networking, defect)

Description

[Treeherder Bug Filer]

Filed by: geoff [at] darktrojan.net
Parsed log: https://treeherder.mozilla.org/logviewer.html#?job_id=304864641&repo=comm-central
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/FDD5Qa61T9qqZPS0X1vn2g/runs/0/artifacts/public/logs/live_backing.log

---

Comment 1

[Geoff Lankow (:darktrojan)]

I've been trying to figure this one out but it's beyond me. I think it's highly likely to be caused by bug 1638358, probably this revision.

The crash happens in CookieCommons::SchemeToSchemeType, which I'd expect to crash because the scheme is mailbox. What happened before? Did the code not get to this point or does schemeIs somehow translate mailbox to file?

Comment 2

[Andrea Marchesini [:baku]]

Is it a specific thunderbird issue? In theory cookies should not be exposed to mailbox URLs. If thunderbird needs this feature, we can treat 'mailbox' schemes as 'file' ones.

Comment 3

[Geoff Lankow (:darktrojan)]

I don't know, I'm just the messenger.

Comment 4

[Andrea Marchesini [:baku]]

In bug 1642971 we added 'ftp' scheme to make a test to pass. But maybe there is a bigger issue.

Comment 5

[Magnus Melin [:mkmelin]]

mailbox: urls should NOT be supporting cookies. That's basically what the crashing test is about to assure: https://searchfox.org/comm-central/rev/b3dc7ad59deb6d0e37499ed167d93c467fb1ccb0/mailnews/base/test/TestMailCookie.cpp#173-175

-> we end up at Mozilla crash reason: MOZ_CRASH(Unsupported scheme type) - https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/netwerk/cookie/CookieCommons.cpp#653

I guess the gtest should just be changed to use something like EXPECT_DEATH or EXPECT_EXIT

Comment 6

[Intermittent Failures Robot]

33 failures in 4949 pushes (0.007 failures/push) were associated with this bug in the last 7 days.

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. **

Repository breakdown:

• comm-central: 33

Platform breakdown:

• linux64: 19
• linux64-shippable: 14

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1642935&startday=2020-06-01&endday=2020-06-07&tree=all

Comment 7

[mac198442]

I would think either add something like this to the mozilla-central code:

diff --git a/netwerk/cookie/CookieCommons.cpp b/netwerk/cookie/CookieCommons.cpp
--- a/netwerk/cookie/CookieCommons.cpp
+++ b/netwerk/cookie/CookieCommons.cpp
@@ -645,13 +645,17 @@ nsICookie::schemeType CookieCommons::Sch
if (aScheme.Equals("http")) {
return nsICookie::SCHEME_HTTP;
}

if (aScheme.Equals("file")) {
return nsICookie::SCHEME_FILE;
}

• if (aScheme.Equals("mailbox")) {
• return nsICookie::SCHEME_UNSET;
• }
• MOZ_CRASH("Unsupported scheme type");
  }

} // namespace net
} // namespace mozilla

or doing this to the current test and adding a separate test to make sure setting a cookie with a mailbox url crashes.

diff --git a/mailnews/base/test/TestMailCookie.cpp b/mailnews/base/test/TestMail
--- a/mailnews/base/test/TestMailCookie.cpp
+++ b/mailnews/base/test/TestMailCookie.cpp
@@ -165,23 +165,18 @@ TEST(TestMailCookie, TestMailCookieMain)
* somewhat contrived - still under development, and needs improving!
*/

// *** mailnews tests

// test some mailnews cookies to ensure blockage.
// we use null firstURI's deliberately, since we have hacks to deal with
// this situation...

• SetACookieMail(cookieService, "mailbox://mail.co.uk/", "test=mailnews");
• GetACookieMail(cookieService, "mailbox://mail.co.uk/", cookie);
• EXPECT_TRUE(CheckResult(cookie.get(), MUST_BE_NULL));
  GetACookieMail(cookieService, "http://mail.co.uk/", cookie);
  EXPECT_TRUE(CheckResult(cookie.get(), MUST_BE_NULL));
  SetACookieMail(cookieService, "http://mail.co.uk/", "test=mailnews");
• GetACookieMail(cookieService, "mailbox://mail.co.uk/", cookie);
• EXPECT_TRUE(CheckResult(cookie.get(), MUST_BE_NULL));
  GetACookieMail(cookieService, "http://mail.co.uk/", cookie);
  EXPECT_TRUE(CheckResult(cookie.get(), MUST_EQUAL, "test=mailnews"));
  SetACookieMail(cookieService, "http://mail.co.uk/",
  "test=mailnews; max-age=0");
  GetACookieMail(cookieService, "http://mail.co.uk/", cookie);
  EXPECT_TRUE(CheckResult(cookie.get(), MUST_BE_NULL));
  }

Comment 8

[mac198442]

Cutting and pasting code from gedit obviously is converting - and + to bullets.

Comment 9

[mac198442]

Attachment: same comment as an attachment

Comment 10

[mac198442]

IN the mozilla-central patch those bullets were supposed to be + and in the second patch for the test they were supposed to be -

Comment 11

[mac198442]

How odd it added the attachment even though it got an error saying it failed to be added.

Comment 12

[mac198442]

I changed my mind on this. I think this test should just be removed. I did not realized it is not even run on Windows which is supposed to be the primary platform. I would think the mozilla-central tests on cookie behavior is all we need and the reason we have this issue is the the toolkit code no longer allows specifying mailbox as a scheme so that part of the test is no longer required.

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: bug1642935_rm_mailcookie.patch

I agree, let's just remove it. To the extent it's testing anything, this is a purely theoretical scenario, which currently would just crash if attempted.

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to mac198442 from comment #12)

I changed my mind on this. I think this test should just be removed. I did not realized it is not even run on Windows which is supposed to be the primary platform.

It used to run on Windows until it was disabled here https://hg.mozilla.org/comm-central/rev/62005c97509ce7ffb9f8c00f1d4d6f7d6486a033 in April 2020.

One of the reasons for the test was making sure that this code didn't get changed and cookies would in fact "work" for Mailnews URLs:
https://searchfox.org/mozilla-central/rev/fac90408bcf52ca88a3dcd2ef30a379b68ab24e2/netwerk/cookie/CookieJarSettings.cpp#245-257

So if you remove the test, please add something to M-C to ensure that it's working as expected with Mailnews URLs. The code quoted above could then also be removed. Note that Mailnews URL are not only mailbox: but also imap:, news: and more, see:
https://searchfox.org/comm-central/source/mailnews/base/util/nsNewMailnewsURI.cpp

Comment 15

[mac198442]

(In reply to Jorg K (CEST = GMT+2) from comment #14)

One of the reasons for the test was making sure that this code didn't get changed and cookies would in fact "work" for Mailnews URLs:
https://searchfox.org/mozilla-central/rev/fac90408bcf52ca88a3dcd2ef30a379b68ab24e2/netwerk/cookie/CookieJarSettings.cpp#245-257

Actually the point was to make sure the the code would NOT wok for mailbox:// URLs But then why test for just that not working what about xyzzy:// URLs or abcdefg:// URLs or mycatsatonthekeyboard:// URLs?

Comment 16

[Jorg K (CEST = GMT+2)]

Right, you stopped mailbox: URLs and others from working, but there is still old code in the system that was part of a scheme that tried to make them work.

Comment 17

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (CEST = GMT+2) from comment #14)

So if you remove the test, please add something to M-C to ensure that it's working as expected with Mailnews URLs.

As of current, attempting to use cookies for a mailnews url would cause a crash - which should stop all such attempts.

How would setting cookie code ever trigger in the wild? You'd have to run js to do it for a mailnews url, but all the content we allow js for is remote.

Comment 18

[mac198442]

(In reply to Jorg K (CEST = GMT+2) from comment #16)

Right, you stopped mailbox: URLs and others from working, but there is still old code in the system that was part of a scheme that tried to make them work.

Well then probably should remove that code., and still really don't need this test.

Comment 19

[mac198442]

There are too many tests that are now perma-failing so no one is paying much attention to may of the tests anymore. This is NOT the desired situation. We need to be back to builds and tests are all supposed to pass.

Comment 20

[mac198442]

To make it clear, the current test ensures you can set a cookie suing a mailmox:// UTL but that the cookie can never be returned using the same URL. That is a really stupid behavior to be testing in the first place.

Comment 21

[mac198442]

To make it clear, the current test ensures you can set a cookie suing a mailbox:// URL but that the cookie can never be returned using the same URL. That is a really stupid behavior to be testing in the first place.

Comment 22

[mac198442]

Either setting the cookie should generate an error or getting the cookie should return the cookie so what is being tested for is illogical behavior.

Comment 23

[mac198442]

To make it 100% clear the test here is because it was formerly possible to specify mailbox as a URL scheme for cookies. so this was meant to make sure that firstly such things will never actually be able to be retrieved, but then also to make sure there was no cross contamination between cookies with a mailbox scheme and those with an http scheme. Now that it is not possible to create a cookie with a mailbox scheme this test is no longer necessary and should just be removed If you are really paranoid then a test to ensure the trying to set a cookie with a mailbox URL scheme fails.

Comment 24

[mac198442]

Oh and if you do add such a test then you lose if you still can't get it to work under windows

Comment 25

[Jorg K (CEST = GMT+2)]

Thanks for the comments. As mentioned in bug 1632115 comment #5, the issue is that NewChannelFromURI() fails here
https://searchfox.org/comm-central/rev/e62a0af3cba6e1c65b2d4be02d3fefce88cf3f8f/mailnews/base/test/TestMailCookie.cpp#44
so we never get to the SetCookieStringFromHttp(), at least on Windows.

So I guess removing the test as suggested is the way forward here. That brings me back to
https://searchfox.org/mozilla-central/rev/fac90408bcf52ca88a3dcd2ef30a379b68ab24e2/netwerk/cookie/CookieJarSettings.cpp#245-257
which should be removed. Do you agree?

Comment 26

[Magnus Melin [:mkmelin]]

Yes that should also be removed.

Comment 27

[mac198442]

It seems this is no longer an issue since the landing of the fix for bug 1642971.

Comment 28

[mac198442]

Seems they realized it is stupid allow a malicious website to force firefox to crash by attempting to set a cookie with an invalid scheme.

It might be interesting to see if with the new code this test will work under WIndows.

Comment 29

[Ben Campbell]

Comment on attachment 9155624 [details] [diff] [review]
bug1642935_rm_mailcookie.patch

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

LGTM
(also, today I learned that the m-c gtests play audio tones! I was very confused, trying to figure out where the odd electrical interference on my speakers was coming from...)

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2a744e5feacb
remove TestMailCookie Gtest since it's testing thing that can't work for real. r=benc

Comment 31

[Intermittent Failures Robot]

24 failures in 5211 pushes (0.005 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 24

Platform breakdown:

• linux64: 14
• linux64-shippable: 10

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1642935&startday=2020-06-08&endday=2020-06-14&tree=all