Closed Bug 1721850 Opened 2 years ago Closed 1 year ago

Crash in [@ nsIconChannel::GetHIconFromFile]

Categories

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

Unspecified
Windows 10
defect

Tracking

()

RESOLVED FIXED
96 Branch
Tracking Status
firefox-esr91 --- disabled
firefox94 --- disabled
firefox95 --- disabled
firefox96 --- fixed

People

(Reporter: sefeng, Assigned: cmartin)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(3 files, 1 obsolete file)

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
Severity: -- → S2

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

Severity: S2 → S4
Priority: -- → P2

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

Assignee: nobody → bobowencode
Status: NEW → ASSIGNED

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.

Flags: needinfo?(nika)

(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.

Flags: needinfo?(nika)
Assignee: bobowencode → cmartin

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.

Attachment #9249870 - Attachment description: Bug 1721850 - Refactor of nsIconChannel → Bug 1721850 - Refactor of nsIconChannel (Part 1)
Attachment #9249871 - Attachment description: Bug 1721850 - Remote Windows nsIconChannel → Bug 1721850 - Refactor of nsIconChannel (Part 2)
Attachment #9249870 - Attachment is obsolete: true

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.

Pushed by cmartin@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch
Blocks: 1750033
See Also: → 1755777
You need to log in before you can comment on or make changes to this bug.