1471089 - Preloaded content processes can interfere with the startup behavior of the script preloader

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Kris Maglione [:kmag]]

The content process script preloader uses the initial load of a content page as the cut-off point for its startup window. When we have a pre-loaded content process, we can run into all sorts of problems.

If we don't load a content document into it early enough at startup, for instance, we can wind up not sending script cache data back to the parent process in time for it to be written to the cache.

If we *do* send the data back in time, we can still run into issues with the next preloaded process, since the second pre-loaded process we start tends to wait a long time for us to start loading something into it. Which means we load many more scripts during the pre-loader's startup window. That's a problem for bug 1470793, since we wind up keeping the encoded data for those scripts in memory forever.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1471089: Improve handling of pre-loaded content processes in script preloader.

Review commit: https://reviewboard.mozilla.org/r/252942/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252942/

Comment 2

[Eric Rahm [:erahm]]

Comment on attachment 8987704 [details]
Bug 1471089: Improve handling of pre-loaded content processes in script preloader.

https://reviewboard.mozilla.org/r/252942/#review259836

Overall looks good, I think you just forgot to call `FinishContentStartup`.

::: js/xpconnect/loader/ScriptPreloader.cpp:389
(Diff revision 1)
>  
>      return NS_OK;
>  }
>  
> +void
> +ScriptPreloader::FinishContentStartup()

It looks like this isn't actually called.

::: js/xpconnect/loader/ScriptPreloader.cpp:491
(Diff revision 1)
> +    auto cleanup = MakeScopeExit([&] {
> +        // If the parent is expecting cache data from us, make sure we send it
> +        // before it writes out its cache file. For normal proceses, this isn't
> +        // a concern, since they begin loading documents quite early. For the
> +        // preloaded process, we may end up waiting a long time (or, indeed,
> +        // never loading a document), so we need an additional timeout.

Thanks for the thorough comment

::: js/xpconnect/loader/ScriptPreloader.cpp:505
(Diff revision 1)
>          return Ok();
>      }
>  
>      MOZ_TRY(mCacheData.init(cacheFile.ref()));
>  
> -    return InitCacheInternal();
> +    MOZ_TRY(InitCacheInternal());

Probably doesn't matter, but is this change necessary?

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8987704 [details]
Bug 1471089: Improve handling of pre-loaded content processes in script preloader.

https://reviewboard.mozilla.org/r/252942/#review259836

> It looks like this isn't actually called.

Urrrrgh. I accidentally deleted a line when I was cleaning up my debugging logs, and accidentally deleted some extra lines. I apparently wasn't seeing clearly when I fixed them, and copied ForceWriteCacheFile() rather than FinishContentStartup() ...

> Probably doesn't matter, but is this change necessary?

Oh, no. I initially had the timer creation after this call, but that missed the cases where we had no cache.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8987704 [details]
Bug 1471089: Improve handling of pre-loaded content processes in script preloader.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252942/diff/1-2/

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8987704 [details]
Bug 1471089: Improve handling of pre-loaded content processes in script preloader.

https://reviewboard.mozilla.org/r/252942/#review259852

Looks good, thanks!

Comment 6

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac67cc017934d225ce663484e1348e77818c0535
Bug 1471089: Improve handling of pre-loaded content processes in script preloader. r=erahm

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ac67cc017934