1130128 - Scripts cached by nsFrameScriptExecutor leak when compiled in the current global

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Billm and I were looking into a docshell leaks-until-shutdown, and noticed that some scripts being held alive by the nsFrameScriptExecutor are entraining a ContentFrameMessageManager, which in turn is holding alive the XPCWN for a docshell, which keeps the doc shell alive.  It looks like some self-hosted code is getting compiled against whatever page happens to load it first, which ends up keeping the docshell for that page alive.

If we compile this cached code (in nsFrameScriptExecutor::TryCacheLoadAndCompileScript) against the junk scope, then the doc shell's XPCWN is no longer being held alive.  (The doc shell is still leaking from C++, so maybe this is a red herring...)

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Compile scripts cached by nsFrameScriptExecutor in the junk scope to avoid leaking the compartment. WIP

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 8560760 [details] [diff] [review]
Compile scripts cached by nsFrameScriptExecutor in the junk scope to avoid leaking the compartment. WIP

Bobby, is PrivilegedJunkScope() the right scope for this do you think?  Or maybe CompilationScope()?  The comment near those explicitly tells you to ask the XPConnect module owner so that's what I'm doing. ;)

I think we don't ever run this code, and with the current set up you end up compiling it in some random scope which leaks the associated compartment.

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8560760 [details] [diff] [review]
Compile scripts cached by nsFrameScriptExecutor in the junk scope to avoid leaking the compartment. WIP

Review of attachment 8560760 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsFrameMessageManager.cpp
@@ +1538,5 @@
>    if (dataStringBuf && dataStringLength > 0) {
>      AutoSafeJSContext cx;
> +    // We compile in the junk scope instead of the current JS object to avoid
> +    // entraining whatever random stuff is on the current global, which could
> +    // be for some random page.  Maybe xpc::CompilationScope() makes more sense?

These cached scripts are always cloned into another compartment before they're run, right? If so then yeah, do xpc::CompilationScope.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Compile scripts cached by nsFrameScriptExecutor in the compilation scope to avoid leaks.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 2 - CompilationScope() is infallible.

I'll attach the trivial -w patch in a moment.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 2 with -w

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8561429 [details] [diff] [review]
part 2 - CompilationScope() is infallible.

Review of attachment 8561429 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the diff -w. ;-)

Comment 8

[Andrew McCreight [:mccr8]]

No problem.  The regular diff is completely awful. ;)

Comment 9

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8d5d816871
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4a114ab2fcca

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a8d5d816871
https://hg.mozilla.org/mozilla-central/rev/4a114ab2fcca