Open Bug 1636664 Opened 7 months ago Updated 5 months ago

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

Categories

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

defect

Tracking

()

Tracking Status
firefox78 --- affected

People

(Reporter: mayankleoboy1, Unassigned)

References

()

Details

(Keywords: crash, nightly-community)

Crash Data

Attachments

(4 files)

Attached file 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.

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

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 ]

Has STR: --- → yes
Component: Graphics: WebRender → Layout
See Also: → 1341489
Crash Signature: [@ nsCSSFrameConstructor::ConstructFrameFromItemInternal ]
Summary: Modified mutate-dl test crashes the tab after loading for a few seconds → stack overflow: Modified mutate-dl test crashes the tab after loading for a few seconds
See Also: → 1636670

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

Severity: -- → S2

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

Flags: needinfo?(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?

Severity: S2 → S3
Flags: needinfo?(svoisen) → needinfo?(aethanyc)
Priority: -- → P3

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

Component: Layout → DOM: Core & HTML
Flags: needinfo?(aethanyc)
Priority: P3 → --
Severity: S3 → --

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

Component: DOM: Core & HTML → JavaScript Engine

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.

Component: JavaScript Engine → DOM: Core & HTML

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

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.

Flags: needinfo?(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.

Flags: needinfo?(emilio)
Severity: -- → S3
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.