1911529 - MOZ_DIAGNOSTIC_ASSERT(equal) (aLoadState should contain the same URI passed to this function.)

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[Jan Jaeschke [:jjaschke]]

The diagnostic assert introduced in Bug 1901139 is responsible for some crash reports.

Steps to reproduce:

• Go to https://coder.com/docs/enterprise
• Scroll to bottom
• Click on the link "Make an edit."
• Crash

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Copying crash signatures from duplicate bugs.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

:edgar, since you are the author of the regressor, bug 1901139, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 4

[Jan Jaeschke [:jjaschke]]

Taking over since Edgar is on leave.

Marking S2 for now, fix incoming likely today.

Comment 5

[Jan Jaeschke [:jjaschke]]

The issue in this specific test case seems to be that the URL in the link contains a /./: https://github.com/coder/coder/edit/main/docs/./enterprise.md

The mentioned assert seems to compare the URI with a coalesced version of the URL (where the /./ has been replaced with /), which fails. I guess the bug here is in nsIURI::Equals(), but I'll do a quick fix in here to make sure the current issue is fixed and can be uplifted.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

It seems that there's a bug in the path coalescing code coming from bug 1904582.
As far as I can tell the problem is with the basename's mLen field - when comparing a normalized url vs a reparsed url the two lengths don't match so equals returns false.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1911529 - nsStandardURL::mBasename.mLen isn't properly updated when coalescing r=#necko

mBasename.mLen is only supposed to cover the length of the name apart from
the extension. When there is an extension, we must subtract 1 + the length of
the extension in order to get the correct basename length.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

[Tracking Requested - why for this release]:
Incorrect URL parsing may trigger asserts and return the wrong basename for a file.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6bce1ffcd280
nsStandardURL::mBasename.mLen isn't properly updated when coalescing r=necko-reviewers,kershaw

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6bce1ffcd280

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1911529 - nsStandardURL::mBasename.mLen isn't properly updated when coalescing r=#necko

mBasename.mLen is only supposed to cover the length of the name apart from
the extension. When there is an extension, we must subtract 1 + the length of
the extension in order to get the correct basename length.

Original Revision: https://phabricator.services.mozilla.com/D218529

Comment 12

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Incorrect URL parsing may trigger asserts and return the wrong basename for a file.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: Follow instructions in bug 1911529 comment 0
• Risk associated with taking this patch: low
• Explanation of risk level: This code is covered by tests and fuzzing.
• String changes made/needed: none
• Is Android affected?: yes

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/8348766df0c7

Comment 14

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Issue is reproducible on a 2024-08-05 Nightly build on Windows 10.
Verified as fixed on Firefox 130.0b2 and Firefox Nightly 131.0a1 on Windows 10, Ubuntu 22, macOS 14.