1721850 - Crash in [@ nsIconChannel::GetHIconFromFile]

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P2)

Description

[Sean Feng [:sefeng]]

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/286187e2-c711-4b76-b049-f337e0210720

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(false) (GetHIconFromFile requires call to SHGetFileInfo, which cannot be used when win32k is disabled.)

Top 10 frames of crashing thread:

0 xul.dll nsIconChannel::GetHIconFromFile image/decoders/icon/win/nsIconChannel.cpp:452
1 xul.dll nsIconChannel::GetHIcon image/decoders/icon/win/nsIconChannel.cpp:692
2 xul.dll nsIconChannel::AsyncOpen image/decoders/icon/win/nsIconChannel.cpp:392
3 xul.dll imgLoader::LoadImage image/imgLoader.cpp:2491
4 xul.dll static nsContentUtils::LoadImage dom/base/nsContentUtils.cpp:3659
5 xul.dll mozilla::dom::Document::PreLoadImage dom/base/Document.cpp:12178
6 xul.dll mozilla::dom::Document::MaybePreLoadImage dom/base/Document.cpp:12225
7 xul.dll nsHtml5SpeculativeLoad::Perform parser/html/nsHtml5SpeculativeLoad.cpp:43
8 xul.dll nsHtml5TreeOpExecutor::RunFlushLoop parser/html/nsHtml5TreeOpExecutor.cpp:585
9 xul.dll nsHtml5ExecutorFlusher::Run parser/html/nsHtml5StreamParser.cpp:179

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Win32k lockdown is an experiment, and I think cmartin has seen this failure in try runs.

Comment 2

[Bob Owen (:bobowen)]

Seems like most of these are being caused by listing resource:// URLs.

Comment 3

[Bob Owen (:bobowen)]

I think we got to this in the meeting, but just to be sure: https://phabricator.services.mozilla.com/D118596#3865440

If I'm making changes here anyway to make it remote for windows as well, I guess that you would like me to change this to work a bit more like PageThumbProtocolHandler as stated in this comment.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to Bob Owen (:bobowen) from comment #3)

I think we got to this in the meeting, but just to be sure: https://phabricator.services.mozilla.com/D118596#3865440

If I'm making changes here anyway to make it remote for windows as well, I guess that you would like me to change this to work a bit more like PageThumbProtocolHandler as stated in this comment.

Looking at my old comment again, I'm actually not sure whether it matters in the old situation, but you may still be able to save some time by using the NS_NewSimpleChannel for Windows. As I noted there, we never actually open the nsIconChannel sync, so it shouldn't matter whether we support that properly, and it's probably not worth worrying about. (There are a few other channel types which behave incorrectly when opened sync, like moz-extension:// channels, so we should be fine).

I think the biggest issue here is likely to be just that the Windows nsIconChannel is a bit more advanced than the GTK one, so it may not be possible to use the exact same strategy to run it. It might be easiest to move the logic for changing the channel returned out to the icon protocol (https://searchfox.org/mozilla-central/rev/9e2239eb75e16204a6186c26f80aaa9723420919/image/decoders/icon/nsIconProtocolHandler.cpp#54-79) to make it cross-platform, and return a completely different channel type in content processes. I don't have a strong opinion anymore as to whether to do that with the existing NS_NewInputStreamChannel approach or with NS_NewSimpleChannel.

Whatever strategy you do, you'll probably want to read the channel in the parent process async, as on Windows it appears as though HICONs are read differently when read async (https://searchfox.org/mozilla-central/rev/9e2239eb75e16204a6186c26f80aaa9723420919/image/decoders/icon/win/nsIconChannel.cpp#520-526), probably to avoid blocking I/O on the main thread.

FYI there are some changes happening to NS_NewSimpleChannel in bug 1706594.

Comment 5

[Chris Martin [:cmartin]]

Attachment: Bug 1721850 - Refactor of nsIconChannel (Part 1)

This refactor makes much of the behavior of Windows nsIconChannel into static
free functions, more similar to Linux. This allows a similar remote-ing
strategy to be used on Windows.

Comment 6

[Chris Martin [:cmartin]]

Attachment: Bug 1721850 - Refactor of nsIconChannel (Part 2)

Comment 7

[Chris Martin [:cmartin]]

Attachment: Bug 1721850 - Remote Windows nsIconChannel

Comment 8

[Chris Martin [:cmartin]]

Attachment: Bug 1721850 - Refactor of nsIconChannel (Part 1)

This refactor makes much of the behavior of Windows nsIconChannel into static
free functions, more similar to Linux. This allows a similar remote-ing
strategy to be used on Windows.

Part 2 of this change will just re-organize everything. This change was
impossible to review with all the moving of things.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b0f4a9e2e3f
Refactor of nsIconChannel (Part 1) r=nika
https://hg.mozilla.org/integration/autoland/rev/5daf8505ca8b
Refactor of nsIconChannel (Part 2) r=nika
https://hg.mozilla.org/integration/autoland/rev/b546cd41e82c
Remote Windows nsIconChannel r=nika

Comment 10

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/0b0f4a9e2e3f
https://hg.mozilla.org/mozilla-central/rev/5daf8505ca8b
https://hg.mozilla.org/mozilla-central/rev/b546cd41e82c