1849219 - Multi-second parent process hangs in 'places::NotifyTitleObservers' calling into 'maybeCacheBaseUrlForVisit'

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Daniel Holbert [:dholbert]]

I've noticed several times today that I'm getting multi-second parent process hangs (which lock up the entire browser, and cause OS-level "application not responding, do you want to wait or force-quit" popups to appear). I just captured one of these hangs in the profiler and it seems to show us spending ~15 seconds in mozilla::places::NotifyTitleObservers::Run.

Nearly a third of that time is a garbage collection, but that seems like it may have been triggered by the JS garbage that the places task was generating, so I don't think it was just bad luck / bad timing.

Here's my profile: https://share.firefox.dev/45e57C3

I also did kill -11 on my Firefox process earlier today when this was locking up my browser entirely (I wasn't able to start the profiler or regain control at all at that point). Here's the crash report that that kill operation generated -- it's also in mozilla::places::NotifyTitleObservers::Run():
bp-a4ff68d6-3b4e-417b-aa05-f71870230817

It feels suspicious that this is happening repeatedly today, when I haven't noticed it before. Seems like this might be a recent regression. I don't have STR, unfortunately; I was looking at completely-different tabs/windows between the two times that I noticed the issue, and I noticed it just because my browser started locking up.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)

Seems like this might be a recent regression.

Aha, here's a clue -- It looks like my performance profile is spending all of its time in maybeCacheBaseUrlForVisit. That seems to be a new function added this week in bug 1828669: https://hg.mozilla.org/mozilla-central/rev/d48c4e5647e4#l2.124 So this is almost certainly a regression from that bug.

(That bug is filed under Firefox View -- note that I do have browser.tabs.firefox-view-next set to true, and I do have several tabs open that were directly viewing about:firefoxview-next, if that happens to be relevant for bug 1828669's code being reachable.)

mkaply / jsudiaman, maybe we need to reconsider the approach in bug 1828669 (maybe even worth a backout if we don't have a quick fix)? Random tens-of-seconds parent-process hangs are pretty catastrophic. I don't know what factors are required to trigger them, but if it's not just me, we may want to even back out bug 1828669 until we've got a mitigation in place or another approach (e.g. batching our de-duping work somehow)...

Comment 2

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

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

Comment 3

[Jonathan Sudiaman [:jsudiaman]]

Some de-duping is being done on the SQL side via GROUP BY, but your description indicates that the front-end de-duping is what's causing the hangs. We don't need to back out the entire patch but let's remove the de-duping by title + base url for now.

Comment 4

[Daniel Holbert [:dholbert]]

Thanks!

Comment 5

[Mark Banner (:standard8)]

Hi Jonathan, I've just left a few comments on the original patch that I think also need addressing. Please can you either address them here, or file follow-ups which will be actioned before we release the code.

Comment 6

[Jonathan Sudiaman [:jsudiaman]]

Attachment: Bug 1849219 - Multi-second parent process hangs in 'places::NotifyTitleObservers' calling into 'maybeCacheBaseUrlForVisit' r?mkaply

Comment 7

[Daniel Holbert [:dholbert]]

I just hit this again on a different machine, several minutes after enabling browser.tabs.firefox-view-next on that machine and looking at the redesigned Firefox View.

In today's case, Firefox Nightly unexpectedly hanged for ~tens of seconds, and then shortly later hanged for several minutes, and this repeated with short periods of everything being fine in between long whole-browser hangs. (I sort-of captured one of these several-minute-hangs in the profiler, but I ended up giving up and killing Firefox while the profiler was capturing the profile; filed bug 1849897 on that.)

Could we either get this patch landed or just back out the original patch ASAP? We've invited folks to dogfood the browser.tabs.firefox-view-next about:config pref (internally via slack at least -- not sure how broadly beyond that), so there's potentially a population of users who are having a confusing/bad time due to this right now, potentially without them having any clue about what's going wrong or what to toggle to get back to a working state.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9144a3b18e20
Multi-second parent process hangs in 'places::NotifyTitleObservers' calling into 'maybeCacheBaseUrlForVisit' r=mkaply,places-reviewers,mak

Comment 9

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/9144a3b18e20