Bugzilla

(In reply to Olli Pettay [:smaug] from comment #16)
> "We expect WorkerGlobalScopeBase derived objects to never outlive their mWorkerPrivate" sounds reasonable, but it means also that we assume there aren't ever any leaks. But leaks do occur, unfortunately, and we shouldn't crash because of them.

That kind of brings us back to the options from comment 10. I understood Andrew is saying "this is an invariant we should enforce" and it is also what our code assumed before (risking UAF on the `mWorkerPrivate` in case of leaks which then we wanted to mitigate in bug 1752120 by nulling them out). So the first patch of this stack basically restores the previous behavior before bug 1752120 (through some re-ordering of calls) but adds the asserts to enforce the (presumed) invariant, which might turn out difficult to achieve.

If we really cannot prevent from leaks and must accept them as a normal and expected state of operations, we will never be able to guarantee the invariant of option 1. That would automatically mean, we loosen the strict coupling of `WorkerPrivate`'s lifecycle with `WorkerGlobalScopeBase` derived objects and need to always check `mWorkerPrivate` before using it, bringing us straight to option 2.

> The stack trace in comment AutoEntryScript::AutoEntryScript is about the generic issue we've had with AutoEntryScript forever that it doesn't have Init() method similarly to AutoJSAPI. Whoever calls AutoJSAPI::Init() is supposed to check the return value, and that returns false for example if the global doesn't have JS object anymore.
> I commented about this also in https://phabricator.services.mozilla.com/D141719

Well, this is one of the probably many consequences of 2. - we must add a meaningful and robust error handling if we always need to check `mWorkerPrivate` before using - if it is not there we need to act accordingly without crashing subsequently. And if we can repair this long standing issue together, even better.

I'd then propose to land https://bugzilla.mozilla.org/attachment.cgi?id=9268485 minus the sentinel asserts asap in order to repair the regression from bug 1752120 which makes us crash much more often than in the past before it hits release and then add additional patches to harden incrementally all accesses to `mWorkerPrivate`.  But I'd want to hear also asuth' opinion on the (non-)possibility to enforce the invariant from option 1, of course.

(In reply to Olli Pettay [:smaug] from comment #16)
> "We expect WorkerGlobalScopeBase derived objects to never outlive their mWorkerPrivate" sounds reasonable, but it means also that we assume there aren't ever any leaks. But leaks do occur, unfortunately, and we shouldn't crash because of them.

That kind of brings us back to the options from comment 10. I understood Andrew is saying "this is an invariant we should enforce" and it is also what our code assumed before (risking UAF on the `mWorkerPrivate` in case of leaks which then we wanted to mitigate in bug 1752120 by nulling them out). So the first patch of this stack basically restores the previous behavior before bug 1752120 (through some re-ordering of calls) but keeping the UAF-safety and adding the asserts to enforce the (presumed) invariant, which might turn out difficult to achieve.

If we really cannot prevent from leaks and must accept them as a normal and expected state of operations, we will never be able to guarantee the invariant of option 1. That would automatically mean, we loosen the strict coupling of `WorkerPrivate`'s lifecycle with `WorkerGlobalScopeBase` derived objects and need to always check `mWorkerPrivate` before using it, bringing us straight to option 2.

> The stack trace in comment AutoEntryScript::AutoEntryScript is about the generic issue we've had with AutoEntryScript forever that it doesn't have Init() method similarly to AutoJSAPI. Whoever calls AutoJSAPI::Init() is supposed to check the return value, and that returns false for example if the global doesn't have JS object anymore.
> I commented about this also in https://phabricator.services.mozilla.com/D141719

Well, this is one of the probably many consequences of 2. - we must add a meaningful and robust error handling if we always need to check `mWorkerPrivate` before using - if it is not there we need to act accordingly without crashing subsequently. And if we can repair this long standing issue together, even better.

I'd then propose to land https://bugzilla.mozilla.org/attachment.cgi?id=9268485 minus the sentinel asserts asap in order to repair the regression from bug 1752120 which makes us crash much more often than in the past before it hits release and then add additional patches to harden incrementally all accesses to `mWorkerPrivate`.  But I'd want to hear also asuth' opinion on the (non-)possibility to enforce the invariant from option 1, of course.