1839621 - Remove raw pointers for Notification codes.

Status: RESOLVED DUPLICATE of bug 1931497

(Core :: DOM: Notifications, defect, P2)

Description

[Eden Chuang[:edenchuang]]

https://searchfox.org/mozilla-central/search?q=symbol:T_mozilla%3A%3Adom%3A%3ANotification&redirect=false

After WorkerRunnable refactoring (bug 1800659) landed, some assumptions are not valid anymore. Raw pointers in Notification codes could cause the problem and need to be fixed.

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

Assuming this is about replacing Notification* with RefPtr<Notification>.

Comment 2

[Eden Chuang[:edenchuang]]

It is a little bit more complicated than just replacing Notification* with RefPtr<Notification>

The root cause is the Notification code assumes that all NotificationWorkerRunnables execute Cancel() After a StrongWorkerRef gets a worker shutdown notification. And NotificationWorkerRunnable::Cancel() does nothing.

However, after bug 1800659, instead of calling Cancel(), call WorkerRun() to release resources. And NotificationWorkerRunnable::WorkerRun() could access some already released resources during WorkerRef::Notify(). So we meet UAF/Crash on these resources accessing.
This means the Notification code does not guarantee that NotificatiionWorkerRunnable's lifecycle is covered by a StrongWorkerRef. The StrongWorkerRef is freed too early in the Notification code.

Bug 1839231 and bug 1850843 change the NotificationWorkerRunnable's behavior to keep it doing nothing once the StrongWorkerRef is released by checking passed in WorkerPrivate's status. However, we should make the Notification code follow the Worker life-cycle policy(Always ensure the WorkerRunnable's life-cycle is covered by a StrongWorkerRef)

Replacing Notification* with RefPtr<Notification> should be one of the goals of this bug.

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

Thanks, that sounds like this needs one who is sufficiently expert in workers and runnables, are you going to work on this in the near future or do you want me to take a look?

Comment 4

[Eden Chuang[:edenchuang]]

Thanks for asking, I will work on it.

Comment 5

[Kagami Rosylight [:saschanaz] (they/them)]

Thanks!

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

Eden thinks this may allow removing NotificationRef.

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

Or maybe bug 1894231 will cover this? 👀

Comment 8

[Eden Chuang[:edenchuang]]

Attachment: WIP: Bug 1839621 - P1 remove CheckedUnsafePtr<WorkerPrivate> in Notification code. r=#dom-worker-reviewers

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

Fixed by bug 1931497.