1636664 - stack overflow: Modified mutate-dl test crashes the tab after loading for a few seconds

Status: NEW

(Core :: DOM: Core & HTML, defect, P3)

Description

[Mayank Bansal]

Attachment: pending.7z

1. create new profile
2. Go to : https://mattwoodrow.github.io/dl-test/dl-test.html?count=20000&layer=flattened&siblings=1
3. Wait 10-20 seconds for the URL to load

ER: page loads
AR: Crash

The tab crashes, and I can see a crash in about:crashes. However, when i try to submit the crash report, it consistently fails.
I am attaching the crash dump report.

Comment 1

[Mayank Bansal]

I dont know in which component I should place this bug, as no crash report is being generated. Putting in WR for now.

Comment 2

[Darkspirit]

KDE, X11, Debian Testing

1. tab crash, main profile+fission:
   bp-f0723488-f321-46dd-8138-811a30200509 09.05.20, 10:21 [@ IPCError-browser | ShutDownKill | __poll ]
   bp-07ceeaf8-3b97-4ea8-b262-dc8d90200509 09.05.20, 10:21 [@ nsCSSFrameConstructor::ConstructFrameFromItemInternal ]
   bp-688e6a44-6c4f-4c63-8a91-cd7330200509 09.05.20, 10:19 [@ IPCError-content | RecvConstructBrowser Null or discarded initial BrowsingContext ]

2. tab crash, main profile+fission:
   bp-de191e5c-d272-4704-af16-4c68a0200509 09.05.20, 10:22 [@ nsCSSFrameConstructor::ConstructFrameFromItemInternal ]

3. tab crash, fresh profile:
   bp-d3c21356-acc3-431d-ab21-c4afb0200509 09.05.20, 10:25 [@ arena_t::MallocSmall | Allocator<T>::malloc | replace_malloc ]

4. tab crash, fresh profile:
   bp-4e00c535-998d-414a-a931-4f9e70200509 09.05.20, 10:26 [@ nsCSSFrameConstructor::ConstructFrameFromItemInternal ]

5. tab crash, fresh profile:
   bp-d4a9e29d-0f32-4f1d-9cff-d65680200509 09.05.20, 10:27 [@ nsCSSFrameConstructor::ConstructFrameFromItemInternal ]

Comment 3

[Darkspirit]

Attachment: ff_asan_log.7904.submitted

Comment 4

[Darkspirit]

Attachment: ff_asan_log.8602.submitted

Comment 5

[Emily McDonough [:alaskanemily]]

This looks like it might be the same crash as bug 1341489

Comment 6

[Rachel Tublitz [:rachel]]

S1 or S2 bugs needs an assignee - could you find someone for this bug?

Comment 7

[Sean Voisen (:svoisen)]

It's an artificial test case and very low volume in the wild, so I'm going to set it to S3. That said, we should investigate.

TYLin: Any thoughts about this stack overflow?

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

mozregression gives me this range.
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=179dae92e4d794e7f45ad080ff01908c80691f31&tochange=40df5dd35fdb7ce3652fe4448ac8961c075c928e

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: create-a-tall-dom-tree-by-script.html

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I was able to reduce the original test case into a simple one attached in comment 9. It's a script that creating a very tall DOM tree of 20000 depth recursively, and we exhaust our stack in the recursive call of Element::BindToTree [1] (see the call stack at comment 3).

Maybe we should limit the depth of a DOM tree? Or the depth or js recursive calls?

Since the callstack is in DOM, I'll change the component to DOM.

[1] https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/dom/base/Element.cpp#1598-1606

Comment 11

[Mirko Brodesser (:mbrodesser-Igalia), offline until April 25h]

Moving to Core -> JavaScript, because it seems JS should detect the maximum call stack size is exceeded (that's what Chrome does, btw).

Comment 12

[Emilio Cobos Álvarez (:emilio)]

You can do the same without JS recursion (just put each iteration in a setTimeout or something, then remove and add the root node).

We have different stack size limits from other browsers. We don't limit the DOM depth, which is the only actionable thing here, I think. But I don't think we want to limit the DOM depth artificially.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Mirko Brodesser (:mbrodesser) from comment #11)

Moving to Core -> JavaScript, because it seems JS should detect the maximum call stack size is exceeded (that's what Chrome does, btw).

Element::BindToTree or another function in that stack trace needs to check for this with CheckRecursionLimit(cx) or so. That's not something the JS engine can do automatically.

Comment 14

[Mirko Brodesser (:mbrodesser-Igalia), offline until April 25h]

You can do the same without JS recursion (just put each iteration in a setTimeout or something, then remove and add the root node).

We have different stack size limits from other browsers. We don't limit the DOM depth, which is the only actionable thing here, I think. But I don't think we want to limit the DOM depth artificially.

From a web-developers view, it would be nice to get some clear error-message, when recursing to deep. Do you know why Gecko's stack size limits differ from other browsers? It seems having the stack size limit low enough would catch this error.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

I don't know the details, but there are different trade-offs here. Jan probably can answer, but there are legit use cases for deep recursion in JS.

The limits are here I think: https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/js/xpconnect/src/XPCJSContext.cpp#1144-1177

As Jan said, we could potentially check the stack limit depth in BindToTree, though making UnbindFromTree fallible is definitely scary and can leave a lot of stuff behind in a broken state, which is not great.

It's relatively easy to overflow the stack on other browsers too, see https://bugs.chromium.org/p/chromium/issues/detail?id=1092212 for a recentish example.

Short of artificially limiting the DOM depth in the same way we limit the layout tree depth I don't see a great solution for this. IIRC we limit DOM depth from the HTML parser, but that of course doesn't help with programatically created DOM.

Comment 16

[Mayank Bansal]

signature seems to have changed.