1712413 - freezing loading brazilian constitution source code

Status: RESOLVED FIXED

(Core :: Layout: Generated Content, Lists, and Counters, defect, P3)

Description

[Matheus Armelin]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:88.0) Gecko/20100101 Firefox/88.0

Steps to reproduce:

Open these web site: https://www.planalto.gov.br/ccivil_03/constituicao/constituicao.htm

Click to see source code page

Actual results:

Freezes the Firefox window and consumes lot of memory system

Expected results:

See the source code of brazilian constitution

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Core::Memory Allocator' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Florian Quèze [:florian]]

I can reproduce, and I captured a profile of it: https://share.firefox.dev/3mQFuCl On my fast macbook, displaying the source code caused 14.4s of jank in the content process. Most of the time is spent in nsCounterList::SetScope.

Emilio, do you think there's anything actionable here? If so, feel free to forward the needinfo. Thanks!

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Looks like an O(n^2) in the counters code? Is it reasonably better on your machine with layout.css.counter-ancestor-scope.enabled=false by any chance?

Comment 4

[Florian Quèze [:florian]]

Yes, with the pref set to false, it goes down to 2.2s instead of 14.4s: https://share.firefox.dev/3j1njsO

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Mats, perhaps you can take a look?

Comment 6

[Mats Palmgren (inactive)]

Does anyone know where does our view-source code lives? (i.e. the part that generates the markup, and what CSS we apply to that)

Comment 7

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/jobs?repo=try&revision=f2a6dc87f37d42f10af80d86592e0bfb2df5cc07

Comment 8

[Mats Palmgren (inactive)]

Attachment: Bug 1712413 - Make finding CSS counter scopes on ancestors faster. r=emilio

Comment 9

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

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

Comment 10

[Florian Quèze [:florian]]

(In reply to Mats Palmgren (:mats) from comment #6)

Does anyone know where does our view-source code lives? (i.e. the part that generates the markup, and what CSS we apply to that)

The css seems to be https://searchfox.org/mozilla-central/source/layout/style/res/viewsource.css, and the code generating the markup seems to be https://searchfox.org/mozilla-central/source/parser/html/nsHtml5Highlighter.cpp and surrounding files.
It's possible to see the generated markup by selecting some of it and using "View Selection Source" in the context menu.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc201411942f
Make finding CSS counter scopes on ancestors faster.  r=emilio

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dea767819680
Backed out changeset fc201411942f for causing hybrid build bustages on nsCounterManager.cpp CLOSED TREE

Comment 13

[Cristian Tuns]

Backed out for causing hybrid build bustages on nsCounterManager.cpp

• Backout link
• Push with failures
• Failure Log

Comment 14

[Mats Palmgren (inactive)]

layout/base/nsCounterManager.cpp(156,51): error: incompatible pointer types assigning to 'nsIFrame *' from 'nsContainerFrame '
[task 2021-10-15T21:26:20.561Z] 21:26:20 INFO - for (auto p = aNode->mPseudoFrame; p; p = p->GetParent())

WTF? That's a compiler bug, right?
It should be perfectly fine to assign a 'nsContainerFrame *' result to an 'nsIFrame *' variable.

What is this "Bp-hybrid" target about anyway?

Comment 15

[Cristian Tuns]

(In reply to Mats Palmgren (:mats) from comment #14)

layout/base/nsCounterManager.cpp(156,51): error: incompatible pointer types assigning to 'nsIFrame *' from 'nsContainerFrame '
[task 2021-10-15T21:26:20.561Z] 21:26:20 INFO - for (auto p = aNode->mPseudoFrame; p; p = p->GetParent())

WTF? That's a compiler bug, right?
It should be perfectly fine to assign a 'nsContainerFrame *' result to an 'nsIFrame *' variable.

What is this "Bp-hybrid" target about anyway?

I'm not sure, maybe Aryx can help you with that.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Bp-hybrid compiles most of the code non-unified. Details in bug 1725125. :Andi could be contacted for more info.

Comment 17

[Mats Palmgren (inactive)]

Oh, so it's a non-unified build that didn't get nsContainerFrame.h included in this file probably. Gotcha, I'll fix that, thanks.

Still, it's a bit sad when you push to Try to check "will this break anything" and everything is green, and then it still breaks... :-(

https://treeherder.mozilla.org/jobs?repo=try&revision=3be1e36150da0c092562f688a05789a77f38c382

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1784b93ac61
Make finding CSS counter scopes on ancestors faster.  r=emilio

Comment 19

[Mats Palmgren (inactive)]

Fwiw, I don't know how I feel about commits being backed out just because they fail a non-unified build.
It seems like that will cause a lot of unnecessary churn in our commit history...
Fixing these build errors is almost always as simple as adding a missing #include,
which takes less than a minute to fix if someone is around...

Comment 20

[Cristina Cozmuta (:CrissCozmuta)]

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

Comment 21

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

The patch landed in nightly and beta is affected.
:mats, 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 22

[Ryan VanderMeulen [:RyanVM]]

I think this can ride the trains. Feel free to scream if you feel strongly otherwise.