1899673 - Port bug 1873973: nsStandardURL incorrectly escapes ^ in the path

Status: RESOLVED WONTFIX

(Thunderbird :: Upstream Synchronization, defect)

Comment 1

[Geoff Lankow (:darktrojan) [OOO, back 7 Apr]]

Attachment: Bug 1899673 - Port bug 1873973: nsStandardURL incorrectly escapes ^ in the path. r=#thunderbird-reviewers

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d9ee919f3545
part 1 - Port bug 1873973: nsStandardURL incorrectly escapes ^ in the path. r=aleca

Comment 3

[Geoff Lankow (:darktrojan) [OOO, back 7 Apr]]

If we run this code to display a message and the URL contains a ^, somehow we end up at the external protocol service trying to save something. That's what this error in the logs is all about. My guess is that somewhere in our code we're not expecting an unescaped ^ and returning an error value to the docShell which decides not to load the URL and throws it to the external helper app service. There's nsImapUrl::EscapeSlashes and UnescapeSlashes which mess about with the caret character, but I've not been successful in changing them to get a working state so far.

If we escape the ^ to %5E before passing it to the docShell, the message loads as intended. But we then end up making a second request to fetch the message from the server, which marks the message as read and fails the test. (I think it's a coincidence that the failing test is browser_markAsRead.js, but it is very handy.)

What does work is replacing kOnlineHierarchySeparatorUnknown with a different character, but I'm unsure if there are any other side-effects that would make it a bad idea.

Comment 4

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

Does replacing the ^ character in escape and unescape slashes with another character that is not allowed in the path work?
Such as { ?

Comment 5

[Geoff Lankow (:darktrojan) [OOO, back 7 Apr]]

That's a good idea but no, it doesn't change anything. I don't think those two functions are called near the problem either.

What's weird is that after the error is logged, the rest of the request continues. If the IMAP code had returned an error I don't think that would happen. But I could be wrong about that.

Comment 6

[Geoff Lankow (:darktrojan) [OOO, back 7 Apr]]

I just closed everything for the day and found 80-something .html.part files on my desktop! I guess the external app helper is at least successful in downloading the request. Also, I've just realised that I've said external protocol service a few times instead of external app service.

Comment 7

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

(In reply to Geoff Lankow (:darktrojan) from comment #5)

That's a good idea but no, it doesn't change anything. I don't think those two functions are called near the problem either.

What's weird is that after the error is logged, the rest of the request continues. If the IMAP code had returned an error I don't think that would happen. But I could be wrong about that.

Unfortunately I don't know enough about IMAP to figure out what could be wrong.
What I expect is happening is that unescapeSlashes should be called somewhere, but it isn't. That leaves ^ characters in the path - and they used to get escaped by the URL parser, but now they don't.

(In reply to Geoff Lankow (:darktrojan) from comment #3)

What does work is replacing kOnlineHierarchySeparatorUnknown with a different character, but I'm unsure if there are any other side-effects that would make it a bad idea.

Or maybe the kOnlineHierarchySeparatorUnknown should be the same as the character we use to replace /?

Comment 8

[Geoff Lankow (:darktrojan) [OOO, back 7 Apr]]

https://searchfox.org/mozilla-central/rev/fc76676f61ee37b4c5420649cad6677164a29405/caps/OriginAttributes.cpp#403

Comment 9

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

(In reply to Geoff Lankow (:darktrojan) from comment #8)

https://searchfox.org/mozilla-central/rev/fc76676f61ee37b4c5420649cad6677164a29405/caps/OriginAttributes.cpp#403

Do these schemes use the path as part of the origin?

Comment 10

[Geoff Lankow (:darktrojan) [OOO, back 7 Apr]]

Yes. imap, mailbox, and news schemes use the everything before the query as the origin. Each mail message is a distinct trust domain, so we use the whole spec for comparison.

I'm just coming to the conclusion that the way to fix this is to add an #if IS_ORIGIN_IS_FULL_SPEC_DEFINED block, then check the scheme and return appropriately. Unless we can return earlier where we have an nsIURI available and can check the scheme there.

Come to think of it, don't all file URIs have distinct origins? Can we leverage how that works somehow?

Comment 11

[Geoff Lankow (:darktrojan) [OOO, back 7 Apr]]

Attachment: Bug 1899673 - Change OriginAttributes::PopulateFromOrigin to handle URLs with origins that are the full spec. r=valentin,#necko-reviewers

Comment 12

[Magnus Melin [:mkmelin]]

Backed out since bug 1873973 was backed out:
https://hg.mozilla.org/comm-central/rev/3f4443a43531586fb17e16ceb183189595e949f7

Comment 15

[Intermittent Failures Robot]

4 failures in 3649 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

** This test has failed more than 150 times in the last 21 days. It should be disabled until it can be fixed. **

Repository breakdown:

• comm-central: 4

Platform and build breakdown:

• linux1804-64-qr: 1
  • debug: 1
• linux1804-64-shippable-qr: 1
  • opt: 1
• linux1804-64-asan-qr: 2
  • opt: 2

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1899673&startday=2024-06-03&endday=2024-06-09&tree=all