1871048 - Site responding with content type `application/http-index-format` causes tab to crash

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Malte Jürgens [:maltejur]]

While working on removing support for application/http-index-format (Bug 1824325), I noticed the following bug. Regression testing suggests it was introduced by Bug 1871048. To reproduce:

1. Set up webserver which responds with a valid directory listing, for example 100: You seem to have gotten the directory listing! and with content type application/http-index-format (One such server is running at https://http-index-format.polar.onl via nginx 1.18.0-6ubuntu14.4)
2. Visit that server

Expected result: We see a directory listing with the content "You seem to have gotten the directory listing!"
Actual result: We get the error "Gah. Your tab just crashed." with not much more information

This only seems to happen on real websites and not on pages like file:/// or resource:///, which use the same content type to display the directory listing. Additionally, every time the crash happens, I see [Parent 45727, IPC I/O Parent] WARNING: process 46877 exited on signal 11: file /home/maltejur/mozilla-unified-3/ipc/chromium/src/base/process_util_posix.cc:265 in the terminal. This is the generated crash report https://crash-stats.mozilla.org/report/index/de3fc8a9-ba5c-42fa-a2dc-f2b020231220.

As we probably want to remove web-facing support for that content type anyways, I am not sure if a direct fix is even required. I still wanted to report this issue though, because maybe it is a wider problem that is not just affecting application/http-index-format.

Comment 1

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

Set release status flags based on info from the regressing bug 1851992

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

For more information, please visit BugBot documentation.

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Testcase

Comment 3

[Masatoshi Kimura [:emk]]

I got this crash report: bp-ca36e60f-64c1-46f0-ba66-ed4ac0231220

Comment 4

[Masatoshi Kimura [:emk]]

Flipping network.send_OnDataFinished works around the issue on Firefox 122+. Firefox 121 is not affected by default.

Comment 5

[Kershaw Chang [:kershaw]]

So, the problem here is that nsHtml5StreamParser::OnDataAvailable could be called from nsIndexedToHTML::OnStopRequest at here.
When nsHtml5StreamListener::OnDataFinished is called, we call nsHtml5StreamParser::OnStopRequest and the state is set to STREAM_ENDED here.
After calling OnDataFinished, HttpChannelChild calls nsIndexedToHTML::OnStopRequest and it causes nsHtml5StreamParser::OnDataAvailable to be called again and we hit this assertion.

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Bug 1871048 - Don't call OnDataFinished on main thread, r=jesup

Comment 7

[Donal Meehan [:dmeehan]]

:kershaw we are near the end of the beta cycle. 122.0b9 builds on 2024-01-12.
Though the patch is pending review, any concerns with timing/risk on an uplift for Fx122?

Comment 8

[Kershaw Chang [:kershaw]]

(In reply to Donal Meehan [:dmeehan] from comment #7)

:kershaw we are near the end of the beta cycle. 122.0b9 builds on 2024-01-12.
Though the patch is pending review, any concerns with timing/risk on an uplift for Fx122?

No, I think this should be reviewed soon and I should be able to request an uplift for Fx122.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb70fac152fd
Don't call OnDataFinished on main thread, r=jesup,necko-reviewers,valentin,sunil

Comment 10

[Kershaw Chang [:kershaw]]

Attachment: Bug 1871048 - Don't call OnDataFinished on main thread, r=jesup

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

Comment 11

[Phabricator Automation]

Uplift Approval Request

• Fix verified in Nightly: yes
• String changes made/needed: N/A
• Is Android affected?: yes
• Explanation of risk level: This is a straightforward change, so low risk
• Code covered by automated testing: no
• User impact if declined: Crash when a web site returns application/http-index-format
• Steps to reproduce for manual QE testing: Load this webstie: https://http-index-format.polar.onl
• Risk associated with taking this patch: Low
• Needs manual QE test: yes

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/bb70fac152fd

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/9a06019252c1

Comment 14

[Daniel Bodea [:danibodea]]

When loading https://http-index-format.polar.onl/ in Nightly v123.0a1 from 2024-01-09 or Beta v122.0b8, the tab immediately crashes the tab and creates a crash report with the same signature. This issue no longer occurs when loading the test page in Nightly v123.0a1 from 2024-01-11 or Beta v122.0b9 (20240111131839) treeherder build taken from here. Testing was performed in Windows 10.

Comment 15

[Kershaw Chang [:kershaw]]

Attachment: Bug 1871048 - Allow to call OnDataFinished on main thread, r=sunil