1749056 - Missing lock in CheckListenerChain()

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P2)

Description

[Randell Jesup [:jesup] (needinfo me)]

There appears to be a missing lock accessing mDelegate in CheckListenerChain(). Found with clang thread-safety analysis.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1749056: Parser cleanup r=hsivonen

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

There's a potential deadlock (ordering) between mDelegateMonitor and nsInputStreamPump here.... ideas?

Comment 3

[Henri Sivonen (:hsivonen)]

Bug 1724101 comment 4 is the reason why there's a mutex instead of an atomic pointer.

That is, the point of the mutex is that it keeps us from destructing the pointee of mDelegate when OnDataAvailable is called on the pointee off-the-main-thread.

Since CheckListenerChain() doesn't operate on the pointee, perhaps we could make the pointer atomic and keep the mutex in general but do the null check in CheckListenerChain() without the mutex relying only on the atomicity, which should be enough for null check even though atomicity isn't enough to hold the pointee alive when calling through the pointer.

Comment 4

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

The severity field is not set for this bug.
:hsinyi, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[Hsin-Yi Tsai (she/her)[:hsinyi]]

Feel free to let me know if we should raise the severity level for better tracking.

Comment 6

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

Parser cleanup r=hsivonen
https://hg.mozilla.org/integration/autoland/rev/bac83c9a2e30773cda4d41bad64b8f9164c17c85
https://hg.mozilla.org/mozilla-central/rev/bac83c9a2e30

Comment 7

[Alex Finder]

== Change summary for alert #33310 (as of Fri, 18 Feb 2022 06:59:22 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
85%	wikipedia fnbpaint	macosx1015-64-shippable-qr	fission warm webrender	298.12 -> 43.67
85%	wikipedia fnbpaint	linux1804-64-shippable-qr	fission warm webrender	372.58 -> 55.54
77%	wikipedia fnbpaint	windows10-64-shippable-qr	fission warm webrender	388.50 -> 88.88
70%	wikipedia FirstVisualChange	linux1804-64-shippable-qr	fission warm webrender	406.67 -> 120.00
70%	wikipedia ContentfulSpeedIndex	linux1804-64-shippable-qr	fission warm webrender	407.33 -> 120.58
...	...	...	...	...
2%	amazon fcp	android-hw-g5-7-0-arm7-shippable-qr	warm webrender	439.92 -> 430.21

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=33310

Comment 8

[Henri Sivonen (:hsivonen)]

This change seems performance-relevant to bug 1726299. Not sure if it's a good idea to say so on that bug while this one remains hidden.