Closed Bug 1471089 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

Details

Attachments

(1 file)

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 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?
Attachment #8987704 - Flags: review?(erahm) → review-
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 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!
Attachment #8987704 - Flags: review?(erahm) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac67cc017934d225ce663484e1348e77818c0535
Bug 1471089: Improve handling of pre-loaded content processes in script preloader. r=erahm
https://hg.mozilla.org/mozilla-central/rev/ac67cc017934
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: