1680977 - Crash in [@ mozilla::ipc::MessageChannel::Send | mozilla::dom::PContentChild::SendStartVisitedQueries | IPC_Message_Name=PContent::Msg_StartVisitedQueries]

Status: VERIFIED FIXED

(Toolkit :: Places, defect, P2)

Description

[Sebastian Zartner [:sebo]]

Crash report: https://crash-stats.mozilla.org/report/index/1ef9719c-cbbc-4066-95f8-5f53f0201206

MOZ_CRASH Reason: MOZ_CRASH(IPC message size is too large)

Top 10 frames of crashing thread:

0 xul.dll mozilla::ipc::MessageChannel::Send ipc/glue/MessageChannel.cpp:968
1 xul.dll mozilla::dom::PContentChild::SendStartVisitedQueries ipc/ipdl/PContentChild.cpp:3267
2 xul.dll mozilla::places::History::StartPendingVisitedQueries toolkit/components/places/History.cpp:2168
3 xul.dll mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/docshell/base/BaseHistory.cpp:32:7'>::Run xpcom/threads/nsThreadUtils.h:534
4 xul.dll mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:732
5 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1200
6 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:87
7 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:327
8 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:309
9 xul.dll nsBaseAppShell::Run widget/nsBaseAppShell.cpp:137

Steps to reproduce the issue:

1. Open https://api.csswg.org/bikeshed/
2. Upload a CSS Bikeshed file (e.g. https://github.com/w3c/csswg-drafts/blob/master/css-syntax-3/Overview.bs)
3. Wait a moment

Sebastian

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I reproduced this under a debugger. The message contains 896 URLs; most of them appear to be copies of the same data URL with a 642771-byte path. The total message size is over half a gigabyte.

Comment 2

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

There's no reason to send a query to the parent process for the visited status of data URLs, as we will never store them in history. We should probably "fix" this by checking nsNavHistory::CanAddURIToHistory before starting any SQL queries or sending messages to the parent process.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1680977 - Remove dead Database::MaxUrlLength(). r=mak

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1680977 - Move nsNavHistory::CanAddURIToHistory to BaseHistory. r=mak

And do some minor improvements (use static prefs etc) while at it.

Depends on D99583

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1680977 - Don't bother sending visited queries for URIs that we never store in history. r=mak

Depends on D99584

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2db8a64f71ba
Remove dead Database::MaxUrlLength(). r=mak

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f74a9a5edbef
Move nsNavHistory::CanAddURIToHistory to BaseHistory. r=mak
https://hg.mozilla.org/integration/autoland/rev/2f43fcc8980c
Don't bother sending visited queries for URIs that we never store in history. r=mak

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1680977 - Add a null-check.

aLink might be null in the parent process, and even though it's only
null when getting URIs via IPC (which should never hit this code path in
a correct execution), we don't trust the child process so this is safer.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32f969ba472c
Add a null-check.

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2db8a64f71ba
https://hg.mozilla.org/mozilla-central/rev/f74a9a5edbef
https://hg.mozilla.org/mozilla-central/rev/2f43fcc8980c
https://hg.mozilla.org/mozilla-central/rev/32f969ba472c

Comment 11

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

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9193092 [details]
Bug 1680977 - Add a null-check.

Beta/Release Uplift Approval Request

• User impact if declined: crashes when a page has lots of links with big data URIs.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Doesn't seem like a common situation, but it's a relatively easy fix, so might be worth taking
• String changes made/needed: none

Comment 13

[Timea Cernea [:tbabos][inactive]]

Reproduced the crash on affected 85.0b4 (64-bit) on Windows 10, macOS 10.15 and Ubuntu 16.04.
Verified-fixed on the latest Firefox Nightly 86.0a1 (2020-12-21)(20201221155804) on the above mentioned OSes.
Waiting for the fix to be uplifted on Beta as well if approved.

Comment 14

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9192829 [details]
Bug 1680977 - Remove dead Database::MaxUrlLength(). r=mak

approved for 85.0b5

Comment 15

[Julien Cristau [:jcristau] (back April 22)]

Attachment: Bug 1680977 - stop defining now-unused PREF_HISTORY_MAXURLLEN* macros. r?emilio

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af9f27bb7080
stop defining now-unused PREF_HISTORY_MAXURLLEN* macros. r=emilio

Comment 17

[Dorel Luca [:dluca]]

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

Comment 18

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1a390bee183e
https://hg.mozilla.org/releases/mozilla-beta/rev/de29dec5f30b
https://hg.mozilla.org/releases/mozilla-beta/rev/2cfcea21d100
https://hg.mozilla.org/releases/mozilla-beta/rev/228a82c70191

Comment 19

[Sebastian Zartner [:sebo]]

Tested right now in 85.0b6 (64-Bit) and it works fine for me while it was still broken in 85.0b5. Thanks a lot for the quick fix! And a Happy New Year to you all!

Sebastian