1927714 - Crash after first render [@ core::option::expect_failed | geckoservo::glue::Servo_ResolveStyle ]

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Faustin]

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

Steps to reproduce:

I can reproduce the bug on internal Vue+Vite project. I can't make a specific PoC.
The bug happens without the Deepl extension (https://addons.mozilla.org/fr/firefox/addon/deepl-translate/) but it's hard to reproduce without it.
My profile is clean with only Deepl extension to reproduce easily the bug.

The crash doesn't happens in a Chromium browser.

1. Refresh the page
2. Select text to make Deepl tool appear.
3. The Tab crash.

If I resize the window or make another interaction (ex. click on a button that change a css class), the crash doesn't happens.

Crash report: https://crash-stats.mozilla.org/report/index/28baef41-91b0-43ed-b335-ebbbe0241029 (Firefox 131) / https://crash-stats.mozilla.org/report/index/e0cb6461-283e-44c2-b52b-662020241029 (Firefox Nightly) / https://crash-stats.mozilla.org/report/index/1cfff220-da81-4a94-90e3-a9c270241029 (Firefox 131 without Vue Router change).

Firefox Nightly Build-ID : 20241028212656

Actual results:

Selecting the text should not make the tab crash. (In my opinion, the text selection is not the issue, it's more a DOM related issue, with the apparition of Deepl tool)

Expected results:

The tab should not be disrupt by the text selection.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'WebExtensions::Untriaged' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Faustin]

The WebExtension is a support to reproduce the bug, not in my opinion the origin of the bug (the bug happened without the extention, but i can't reproduce it without)

Comment 3

[Francesco Lodolo [:flod]]

Trying to move to CSS Parsing and Computation, since most of the bugs associated to the crash are about stylo. Please move if there's a better component.

Comment 4

[Mayank Bansal]

Thanks for filing the bug!

1. Can you use mozregression (https://mozilla.github.io/mozregression/) gui to bisect and find the exact Firefox change that causes the crash for you?
2. Alternately, please consider sharing the app privately to a mozilla dev.

Without a testcase and/or a bisection, it would be very difficult to diagnose this crash.

Comment 5

[Faustin]

Attachment: testcase.zip

Comment 6

[Faustin]

I work on a testcase without any internal data.
The testcase contain two javascript file that I wasn't able to minify. Consequently, you need to distribute it inside a simple HTTP server to prevent any protocol error.

Comment 7

[Mayank Bansal]

I served the testcase via a server, installed DeepL (but did not signup/login) and selected a word to bring up the floating button. But I couldnt repro the crash.

Do you have any specific setting or set any specific preferences from about:config? Can you reproduce the crash on a fresh Nightly profile (run Firefox -- p from the command line prompt).

Comment 8

[Faustin]

Attachment: source-map.zip

The sources map associated with the testcase

Comment 9

[Mayank Bansal]

Attachment: I cannot reproduce the crash.mp4

Comment 10

[Faustin]

Thanks for your try to repro. I can reproduce it with this exact testcase and Deepl (link to Crash Report).
Step I use:

1. Creation of a new profile in about:profiles
2. Install Deepl extension, and pass the whool setup but don't login. Give the autorisation to read/write website.
3. Serve the files with python -m http.server (but it was the same with Nginx)
4. Double click on "Demo" after the loading of the index. But if I select normally a word, it seem to crash too.
5. The tab crash

I check my about:config. Nothing special in the fresh profile.

I was able te reproduce it with this exact testcase and Deepl extension on ESR 115.13.0, on a HyperV Debian 12 VM.
Information that I obtain by making the testcase:

• Their is something strange with the tree "Aucun élément", the DOM element appear only after selection. This behaviour doesn't happen in Chromium
• If I remove the document.querySelector("#demo").getBoundingClientRect();. The tree "Aucun élément" appear on load and no crash happen.

Comment 11

[Faustin]

Attachment: Repro_Crash_Nightly.mp4

Comment 12

[Mayank Bansal]

(In reply to Faustin from comment #10)

Thanks for your try to repro. I can reproduce it with this exact testcase and Deepl (link to Crash Report).
Step I use:

1. Creation of a new profile in about:profiles
2. Install Deepl extension, and pass the whool setup but don't login. Give the autorisation to read/write website.
3. Serve the files with python -m http.server (but it was the same with Nginx)
4. Double click on "Demo" after the loading of the index. But if I select normally a word, it seem to crash too.
5. The tab crash

In a new Nightly profile, I serve the attached testcase via a simple http server. I do not see ** "Demo" or any other thing. It is just an empty page for me. Maybe thats why i dont get the crash.
Even on Chrome, I do not see "demo", the tab is just empty.
I dont know why the testcase does not show "demo" and other things for me.
Do I need anything else (any js or library or something) apart from the testcase?

Comment 13

[Faustin]

(In reply to Mayank Bansal from comment #12)

(In reply to Faustin from comment #10)

Thanks for your try to repro. I can reproduce it with this exact testcase and Deepl (link to Crash Report).
Step I use:

1. Creation of a new profile in about:profiles
2. Install Deepl extension, and pass the whool setup but don't login. Give the autorisation to read/write website.
3. Serve the files with python -m http.server (but it was the same with Nginx)
4. Double click on "Demo" after the loading of the index. But if I select normally a word, it seem to crash too.
5. The tab crash

In a new Nightly profile, I serve the attached testcase via a simple http server. I do not see ** "Demo" or any other thing. It is just an empty page for me. Maybe thats why i dont get the crash.
Even on Chrome, I do not see "demo", the tab is just empty.
I dont know why the testcase does not show "demo" and other things for me.
Do I need anything else (any js or library or something) apart from the testcase?

I just check from a fresh downloaded zip, and I serve the attached testcase from testcase directory, so index.html is at the root. I doesn't work when you access index.html from /testcase/index.html

Comment 14

[Mayank Bansal]

Attachment: testcase at root- cannot repro.png

This is with the testcase at root. Unfortunately, "demo" still does not appear for me. Unless you have some other thought, this is the most I can do here.
I will needinfo the relavent mozilla devs, and maybe they will have better luck

Comment 15

[Mayank Bansal]

Emilio, you may have some opinion on this crash report with an active reporter and a testcase.

Comment 16

[Emilio Cobos Álvarez [:emilio]]

I can repro if I start with a french locale / configure language settings to french, then double-click "Historique", which selects the text and triggers the extension.

Comment 17

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

The bug has a crash signature, thus the bug will be considered confirmed.

Comment 18

[Emilio Cobos Álvarez [:emilio]]

Attachment: Reduced test-case.

I could catch in on rr and I understand the issue now.

The issue is that setting the slot name doesn't invalidate style if it ends up mutating the flat tree, which is wrong. If you set crash = false, the second child doesn't show up even though it should.

Comment 19

[Mayank Bansal]

Bisection:
Bug 1712140 - Part 6: Unblock Declarative ShadowDOM tests. r=dom-core,hsivonen
Differential Revision: https://phabricator.services.mozilla.com/D193677

Comment 20

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1927714 - Missing style invalidation in ShadowRoot::AddSlot(). r=smaug

Usually RemoveSlot takes care of this, but if it's not the first slot it
doesn't.

Comment 21

[Emilio Cobos Álvarez [:emilio]]

(In reply to Mayank Bansal from comment #19)

Bisection:
Bug 1712140 - Part 6: Unblock Declarative ShadowDOM tests. r=dom-core,hsivonen
Differential Revision: https://phabricator.services.mozilla.com/D193677

Not really a regression from that bug, I can build a test-case without declarative shadow dom too :)

Comment 22

[Emilio Cobos Álvarez [:emilio]]

Attachment: Test-case without declarative shadow dom.

Comment 23

[Mayank Bansal]

This bug is truly old. This is the bisection range I got with the testcase in comment #22 :
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=db78be8dbc1c81844eb7d35c1a3073078eb5d923&tochange=89d79c2258be1c420153e1adc54338b0165fc88e
so maybe bug 1464428?

Comment 24

[Emilio Cobos Álvarez [:emilio]]

Almost surely bug 1460069 :)

Comment 25

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

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15c40e7e68df
Missing style invalidation in ShadowRoot::AddSlot(). r=smaug

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/48901 for changes under testing/web-platform/tests

Comment 28

[amarc]

https://hg.mozilla.org/mozilla-central/rev/15c40e7e68df

Comment 29

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

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox133 to wontfix.

For more information, please visit BugBot documentation.

Comment 30

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9434206 [details]
Bug 1927714 - Missing style invalidation in ShadowRoot::AddSlot(). r=smaug

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Very old bug but affecting some sites in the wild as per comment 0
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): One-liner
• String changes made/needed: none
• Is Android affected?: Yes

Comment 31

[Faustin]

Sincere thanks Emilio for the quick bugfix!
I can confirm that the fix works on my internal app with the last Nightly build.
Hope that I have done everything right to help you, the Mozilla contribution process is not easy for a first time.

Comment 32

[Donal Meehan [:dmeehan]]

Comment on attachment 9434206 [details]
Bug 1927714 - Missing style invalidation in ShadowRoot::AddSlot(). r=smaug

Approved for 133.0b3

Comment 33

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/d80f5290f6f3

Comment 34

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 35

[Emilio Cobos Álvarez [:emilio]]

Yes, you did great, thanks!

Comment 36

[Donal Meehan [:dmeehan]]

Comment on attachment 9434206 [details]
Bug 1927714 - Missing style invalidation in ShadowRoot::AddSlot(). r=smaug

Approved for 128.5esr.

Comment 37

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/024738fe527a