1416066 - Use ScriptPreloader cache for Activity Stream for possible browser startup win

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement, P2)

Description

[Dan Mosedale (:dmosedale, :dmose)]

The JavaScript startup bytecode cache (see https://groups.google.com/forum/#!msg/mozilla.dev.platform/4u6EsaDC7kY/1TxDr3-BAgAJ for details) has landed.

From https://bugzilla.mozilla.org/show_bug.cgi?id=1405738#c8: 

> The detail is in Bug 900784 and its dependent bugs. To make it short:
>  1. This removes on average 43 ms of every page load. (while being effective
>     on ~50% of the page loads)
>  2. Talos benchmarks reports a 15% improvements on google.com.
>  3. On a personal Window computer with PGO, my personal facebook page load
>     improved from 641ms to 522ms.

Super cool!

It does not, however, currently work for about:pages.  If it did work for about:home, it could potentially make a noticeable dent in browser startup time.

:nbp said the following regarding about pages:

> I would not expect the JSBC to be used in such cases because JavaScript
> files are loaded from resource:// and chrome:// protocols which are not
> implementing the nsICacheInfoChannel API as the necko cache does.

> Thus, if we want to do so, we should probably pre-compiled the content of
> the JSBC and insert it in the OmniJar, and implement the nsICacheInfoChannel
> to let the ScriptLoader know about these pre-compiled resources.

Comment 1

[Jason Orendorff [:jorendorff]]

Nicolas, can you take this and/or put it in the right Bugzilla component?

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Mosedale (:dmose) from comment #0)
> > I would not expect the JSBC to be used in such cases because JavaScript
> > files are loaded from resource:// and chrome:// protocols which are not
> > implementing the nsICacheInfoChannel API as the necko cache does.

This would be a Networking issue.

> > Thus, if we want to do so, we should probably pre-compiled the content of
> > the JSBC and insert it in the OmniJar, and implement the nsICacheInfoChannel
> > to let the ScriptLoader know about these pre-compiled resources.

This would be a Build System & Networking issue.

So depending on how this "cache" would be implemented, and populated, we might have different approaches to solve the problem.

In the mean time, there is nothing required from JavaScript component, except for the completion of Bug 900784 issues.

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> (In reply to Dan Mosedale (:dmose) from comment #0)
> > > I would not expect the JSBC to be used in such cases because JavaScript
> > > files are loaded from resource:// and chrome:// protocols which are not
> > > implementing the nsICacheInfoChannel API as the necko cache does.
> 
> This would be a Networking issue.

Not sure what issue you have in mind.  You are using the http cache (through the cache*channel interface) to store the bytecode.   If you want resource: and chrome: channels do the similar thing, using the same API, someone has to implement it, but the end storage does not necessarily has to be the http cache - at least I would not suggest that as it's kinda volatile and too "http".

> 
> > > Thus, if we want to do so, we should probably pre-compiled the content of
> > > the JSBC and insert it in the OmniJar, and implement the nsICacheInfoChannel
> > > to let the ScriptLoader know about these pre-compiled resources.
> 
> This would be a Build System & Networking issue.
> 
> So depending on how this "cache" would be implemented, and populated, we
> might have different approaches to solve the problem.
> 
> In the mean time, there is nothing required from JavaScript component,
> except for the completion of Bug 900784 issues.

and what is the startup cache then?

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > (In reply to Dan Mosedale (:dmose) from comment #0)
> > > > I would not expect the JSBC to be used in such cases because JavaScript
> > > > files are loaded from resource:// and chrome:// protocols which are not
> > > > implementing the nsICacheInfoChannel API as the necko cache does.
> > 
> > This would be a Networking issue.
> 
> Not sure what issue you have in mind.  You are using the http cache (through
> the cache*channel interface) to store the bytecode.   If you want resource:
> and chrome: channels do the similar thing, using the same API, someone has
> to implement it, but the end storage does not necessarily has to be the http
> cache - at least I would not suggest that as it's kinda volatile and too
> "http".

I thought this was the right component, as the protocol are declared under the netwerk directory.

> > 
> > > > Thus, if we want to do so, we should probably pre-compiled the content of
> > > > the JSBC and insert it in the OmniJar, and implement the nsICacheInfoChannel
> > > > to let the ScriptLoader know about these pre-compiled resources.
> > 
> > This would be a Build System & Networking issue.
> > 
> > So depending on how this "cache" would be implemented, and populated, we
> > might have different approaches to solve the problem.
> > 
> > In the mean time, there is nothing required from JavaScript component,
> > except for the completion of Bug 900784 issues.
> 
> and what is the startup cache then?

The start-up cache being discussed here, is the ScriptLoader glue which connects XDR with the Alternative Data API.

What is missing is the Alternative Data API for the channels provided for the resource: and chrome: protocols. The ScriptLoader and XDR should be the same.

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> I thought this was the right component, as the protocol are declared under
> the netwerk directory.

Ah, if it's about a component to file bugs at then you are right.  Yes, it's net code.

> What is missing is the Alternative Data API for the channels provided for
> the resource: and chrome: protocols. The ScriptLoader and XDR should be the
> same.

Good to know.

Comment 6

[Honza Bambas (:mayhemer)]

I don't think this is a networking but at all.  Everything that has been requested for binding JSBC with an http response has been done (API + implementation).  I think what you want is to turn this bug to a meta bug and file necessary particular changes in respective components, if there is anything we need to do on the net side.

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> if there is anything we need to do on the net side.

My understanding was that we needed the equivalent of a "cache" for the about: pages, such that the ScriptLoader can store the btyecode in it.

Which component would that be if this is not the network component?

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> (In reply to Honza Bambas (:mayhemer) from comment #6)
> > if there is anything we need to do on the net side.
> 
> My understanding was that we needed the equivalent of a "cache" for the
> about: pages, such that the ScriptLoader can store the btyecode in it.
> 
> Which component would that be if this is not the network component?

Depends on where the implementation is going to happen.  If inside respective channels, then probably with respect to a component a channel is implemented at.

Is this about "about:*" pages or "resource:" and "chrome:"?  

There is a number of about handlers, each usually with its own implementation of the channel, scattered around the platform/products.  resource: is in network but usually just substs to file:.  I don't recall where chrome: protocol code is but usually just substs to file: as well internally.

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Honza Bambas (:mayhemer) from comment #8)
> (In reply to Nicolas B. Pierron [:nbp] from comment #7)
> > Which component would that be if this is not the network component?
> 
> Depends on where the implementation is going to happen.  If inside
> respective channels, then probably with respect to a component a channel is
> implemented at.
> 
> Is this about "about:*" pages or "resource:" and "chrome:"?  

Sorry this is about the resource:* and chrome:* URL, which are the URL of of the JavaScript files.

> There is a number of about handlers, each usually with its own
> implementation of the channel, scattered around the platform/products. 
> resource: is in network but usually just substs to file:.  I don't recall
> where chrome: protocol code is but usually just substs to file: as well
> internally.

Do we open another channel for file:* URL? If so we would have to implement it for file:* and forward the nsICacheInfoChannel it to the resource:* and chrome:* channel.

(In the mean time this does not belong in the JS component)

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> Do we open another channel for file:* URL? 

Yep, internally we map resource and chrome URLs to file URLs and load it with a file channel (don't remember exact details).  The result URL remains resource or chrome.

> If so we would have to implement
> it for file:* and forward the nsICacheInfoChannel it to the resource:* and
> chrome:* channel.

Hmm..  I don't know.  It's too low, but maybe that is the place.  I'm just a bit afraid it's too generic thing to do...  

Not sure how complicated this should be as well.  You somehow have to give the file channel on the parent process a reference to some storage space in the user's local part of the profile (can be a simple file).  And it also has to expire with every Firefox update (or be deleted as part of the update process), which is probably going to be the hardest part to do.  OTOH, startup cache has to expire as well, hence it might be the place?  If this is going to happen in higher levels like resource: or chrome: channel, we may end up with very similar solution, though.  Hence, file: might be the place.

And btw, not sure I've already raised that, but isn't startup cache just about this bug?  I don't recall who knows the startup cache the best these days, but we should definitely ask before we start writing the code with what has been so far suggested!

CC'ing bkelly in case he may have some ideas or comments what would be the best approach as he worked on blob remoting touching file loading as well.

Ben, in a nut shell: let nsIFileChannel implementation on the child implement nsICacheInfoChannel providing a way to store and then request alternative data stream (in this case a JS compiled byte code).  This probably means to look for a local-profile-located file (or something) based on e.g. hashed name of the file (or result principal) URL.  If found, open that file for reading as an alt data stream, when requested.  On alt data write, do the same but opposite way - for writing.

see https://dxr.mozilla.org/mozilla-central/rev/574f4f58fe09dd590ea892406e237318c31705b4/netwerk/base/nsICacheInfoChannel.idl#79-104 for details of the API.

> 
> (In the mean time this does not belong in the JS component)

file is in network.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> CC'ing bkelly in case he may have some ideas or comments what would be the
> best approach as he worked on blob remoting touching file loading as well.

Adding Andrea, since I think that is who you are thinking of. :-)

I think the root problem here is that we don't have any caching for these kinds of URLs.  So where do we store the alternate data stream?  How does it get evicted on disk pressure? etc

Right now http cache supports alternate data stream.  We are adding the alternate data stream to Cache API as well.

It sounds like we need a new caching mechanism or new interface to http cache for these kinds of chrome URLs.  It would offer the alternate data stream storage option, but would just pass through the main resource load to the original disk storage.

Comment 12

[Nicolas B. Pierron [:nbp]]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> And btw, not sure I've already raised that, but isn't startup cache just
> about this bug?

John, any idea how we do the invalidation of the startup cache these days? Also, any idea if we have a storage space like the startup cache for resource:* and chrome:* URL, such that we can benefit from the ScriptLoader caching, in pages such as about:newtab.

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Ben Kelly [:bkelly] from comment #11)
> (In reply to Honza Bambas (:mayhemer) from comment #10)
> > CC'ing bkelly in case he may have some ideas or comments what would be the
> > best approach as he worked on blob remoting touching file loading as well.
> 
> Adding Andrea, since I think that is who you are thinking of. :-)

Ah!  Yes, that is the one, thanks.

> 
> I think the root problem here is that we don't have any caching for these
> kinds of URLs.  So where do we store the alternate data stream?  How does it
> get evicted on disk pressure? etc

Yes, good questions.  Where - separate file in the local part of the user profile.  Evict on pressure?  Don't evict!

> 
> Right now http cache supports alternate data stream.

HTTP cache implements an API that has been designed to be consumed primarily by HTTP channels.  HTTP channels implement a higher level API (nsICacheInfoChannel) that the script loader is a consumer of.  This is the API that resource and chrome channels (or the inner file channel) should implement.

>  We are adding the
> alternate data stream to Cache API as well.

Can you please provide a bug #?  DOM Cache has nothing to do with HTTP cache...

> 
> It sounds like we need a new caching mechanism 

Probably.

> or new interface to http
> cache 

Absolutely not.  Don't touch HTTP cache please.  It's purposed primarily for HTTP content caching and not whatever other stuff that happens to need 'expiration on low disk pressure'.  That is the only thing that caching for alt-data in resource and chrome would benefit from when using HTTP cache directly, but nothing else.  Also, this would remove HTTP content from the cache way more likely (shared limit) which would degrade web page load performance.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> >  We are adding the
> > alternate data stream to Cache API as well.
> 
> Can you please provide a bug #?  DOM Cache has nothing to do with HTTP
> cache...

See bug 1350359 and bug 1336199.  Our plan is to provide an a separate implementation of nsICacheInfoChannel specific to Cache API.  It shouldn't impact http cache at all, AFAIK.

Comment 15

[Jay Lim [:imjching]]

I looked into the possibility of pursuing this issue and here are my findings:

Looked into `about:newtab` specifically since that uses a lot of JavaScript (React), though I'm not too sure if there are other `about:*` pages which utilize more JavaScript than what Activity Stream uses.

All of the JavaScript code loaded from the `resource` protocol on `about:newtab` was saved locally and served from the `http` protocol. The goal was to use the existing bytecode cache implementation and find out how much this helps on every page load.

Here are the profiles:

nsJSUtils::ExecutionContext::CompileAndExec: https://perfht.ml/2IKlkbo (without bytecode cache) - 71ms
nsJSUtils::ExecutionContext::DecodeAndExec: https://perfht.ml/2rS96DZ (with bytecode cache) - 58ms

I searched for `mozilla::dom::ScriptLoader::EvaluateScript` because that function contains the logic to either load from the bytecode cache or parse the script. (https://searchfox.org/mozilla-central/source/dom/script/ScriptLoader.cpp#2278-2351)

Let's get some ratios of page load time with JSBC to page load time without JSBC.

Looking at the results section here (https://blog.mozilla.org/javascript/2017/12/12/javascript-startup-bytecode-cache/), we have a ratio of about 0.92 for Wikipedia and 0.88 for Facebook Timeline. I am not exactly sure if the results were calculated based on EvaluateScript, but I don't think it matters since we are taking ratios here. 

Going back to our profiles, the ratio that we have is 0.82, which is a pretty reasonable estimate if we were to compare it with Facebook Timeline since both Activity Stream and Facebook Timeline use React.

To implement JSBC for `about:*` pages, particularly for resource and chrome procotols, it seems like there is quite a decent amount of work that needs to be done. At the moment, the bytecode cache is stored in the network cache. There is a patch that allows these alternative data types to be stored in the DOM cache.

:mayhemer said the following regarding the implementation:

> I think we should split out the alt-data READING support out off the nsICacheInfoChannel interface to its own new interface to make this easier (preferAlternativeDataType, preferredAlternativeDataType, alternativeDataType)
> then let res and chrome channel work with it.  it's up to the channel impl to find a location of the alt data (according the preferredAlternativeDataType) and serve it instead of what the channel does now.  there is no input stream hanging of off the nsICacheInfoChannel inteface, because there doesn't need to be one (that's what migth cofuse you)
> it's up to the channel to intenally fetch the right data, using whatever techniques (probably pump reading an nsIFileInputStream or something - I don't know the internals of how the alt data for chrome and res are stored and where)
> note that channels have a contract to obey: consumer calls asyncOpen(listener) on a channel, and when it succeeds, it then MUST call onStartRequest, onDataAvailable, onStopRequest on it, see nsIStreamListeer interface
> let's call the new inteface nsICacheAltDataProvider?

After discussing with :mconley IRL, it looks like it is not worth pursuing this bug for now since it only saves about 10ms (and maybe more, but the ratio should be around 0.8).

Comment 16

[Jay Lim [:imjching]]

:dmose, what are your thoughts on this?

Also, on your first comment, you mentioned that it removes on average 43ms of every page load. Out of curiosity, where did you get this measurement from?

I suspect that it's on average 43ms for medium/large websites, but it might not be the case for something like `about:newtab`, though I might be wrong on that.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jay Lim [:imjching] from comment #15)
> :mayhemer said the following regarding the implementation:
> 
> > I think we should split out the alt-data READING support out off the nsICacheInfoChannel interface to its own new interface to make this easier (preferAlternativeDataType, preferredAlternativeDataType, alternativeDataType)
> > then let res and chrome channel work with it.  it's up to the channel impl to find a location of the alt data (according the preferredAlternativeDataType) and serve it instead of what the channel does now.  there is no input stream hanging of off the nsICacheInfoChannel inteface, because there doesn't need to be one (that's what migth cofuse you)
> > it's up to the channel to intenally fetch the right data, using whatever techniques (probably pump reading an nsIFileInputStream or something - I don't know the internals of how the alt data for chrome and res are stored and where)
> > note that channels have a contract to obey: consumer calls asyncOpen(listener) on a channel, and when it succeeds, it then MUST call onStartRequest, onDataAvailable, onStopRequest on it, see nsIStreamListeer interface
> > let's call the new inteface nsICacheAltDataProvider?

Splitting it out from nsICacheInfoChannel would probably make the service worker effort a bit harder.   We could adapt, but I'd prefer not to change it right now if its not necessary.

Also, we are going to be getting other benefits by implement nsICacheInfoChannel more fully in service workers.  For example, we can get better image cache behavior by providing a stable cache key for things served from Cache API.  Its possible about:newtab would benefit from this as well.

I realize you're not moving forward for now.  I just wanted to raise my concern about this point.

Comment 18

[Kris Maglione [:kmag]]

For the specific case of about:newtab, I'm not sure comparing it to remote pages is really a valid way to estimate the wins. There are too many differences in the way http: and resource: loads work.

That said, I think we may be better off just trying to use the ScriptPreloader cache for resource: loads. That should be relatively simple to implement for this use case, I think. It has the disadvantage of only supporting synchronous loads at the moment, but it has the advantage of starting the decoding early during process startup, and knows how to decide which scripts to decode based on content process type. Which means we'd have the decoded scripts available much sooner for Activity Stream processes (when they get their own process type) without affecting other processes.

Comment 19

[Nicolas B. Pierron [:nbp]]

(In reply to Jay Lim [:imjching] from comment #15)
> I searched for `mozilla::dom::ScriptLoader::EvaluateScript` because that
> function contains the logic to either load from the bytecode cache or parse
> the script.
> (https://searchfox.org/mozilla-central/source/dom/script/ScriptLoader.
> cpp#2278-2351)

This only account for the parsing and initialization code. The JSBC is caching code executed even after each script first execution. Thus we should also include any event handler after these, such as the onload events.

> Looking at the results section here
> (https://blog.mozilla.org/javascript/2017/12/12/javascript-startup-bytecode-
> cache/), we have a ratio of about 0.92 for Wikipedia and 0.88 for Facebook
> Timeline. I am not exactly sure if the results were calculated based on
> EvaluateScript, but I don't think it matters since we are taking ratios
> here. 

They were computed by measuring the wall clock between the navigation start and the onload event.
These results are including everything which is not JS related. The addon used to get these numbers is linked below the results.

Note, the JSBC is saving the bytecode when the page becomes idle, after the onload event.

(In reply to Jay Lim [:imjching] from comment #16)
> Also, on your first comment, you mentioned that it removes on average 43ms
> of every page load. Out of curiosity, where did you get this measurement
> from?

This comes from the TIME_TO_LOAD_EVENT_END_MS telemetry results made in July 2017 on a few percent of our Nightly population:
https://docs.google.com/spreadsheets/d/1pXtSdDmY4aneEmTgVSiqaK_g9R-C1OqHJSf9PPEYiko/edit#gid=503801958 (Bug 900784)

Here, "On average" also includes the time where the JSBC is not used, and also when it encodes the bytecode.

Comment 20

[Jay Lim [:imjching]]

Attachment: prototype-activity-stream.diff

Comment 21

[Jay Lim [:imjching]]

Attachment: prototype-mozilla-central.diff

Comment 22

[Jay Lim [:imjching]]

I revisited this bug again and looked into the possibility of pursuing this issue for Activity Stream using the *ScriptPreloader cache*. Based on my findings, it seems like there is a possible win for Activity Stream. There are two parts to this:

1. Getting the script into the ScriptPreloader cache, which is the scriptCache files.

For this, there are two ideas:
a) We will note the script asynchronously whenever a new content process starts up. For this to work, we will need to ensure that the script noting method is executed before the "document-element-inserted" event because that finalizes the script caching process and stops noting any new scripts. (See https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/js/xpconnect/loader/ScriptPreloader.cpp#352-361.)

b) Put Activity Stream's scripts into the scriptCache file of the content process during build time. This file will act as our base cache file. This means that whenever the cache is invalidated, we will replace the existing scriptCache file with this initial base file instead of writing an empty cache file. The main part to this is figuring out how to prepopulate the scriptCache file with those scripts during build time. With this approach, we can avoid noting the scripts manually like in part (a).

One drawback to both of these ideas is that the approaches will attempt to parse the scripts and compile them for the case when someone opens a non-about:newtab link in a new tab (and this creates new content process). If that user did not navigate to the about:newtab page, then the decoding process would be wasteful.

2. Reading the script from the ScriptPreloader cache.

There are also two ideas for this:
a) Use mozJSSubScriptLoader to load Activity Stream scripts into the about:newtab window instead of using the regular <script> tags. The implementation is already there: Services.scriptloader.loadSubScript(file, window). I am not sure what was the original intention of mozJSSubScriptLoader and what it is used for, so I do not know if this is the right choice to load those scripts into the content process through the main process.

b) Use <script> tags to load those JS files. For this to work, we will need to modify EvaluateScript() in ScriptLoader so that it does not attempt to parse the script when it is already in the ScriptPreloader cache. We will also need to avoid opening the file again when we already have the script in the cache. This means that we might need to modify this part: https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/dom/script/ScriptLoader.cpp#1175-1190. (Probably something along the lines of ScriptPreloadHandler.) Also, I don't think we need to handle SRI for local resources, but since the SRI code is tightly coupled with ScriptLoader, this also suggests that ScriptLoader might not be a good place to use the ScriptPreloader cache.

--

Prototype:

I have attached two patches for the prototype that I have worked on: one for Activity Stream and another one for mozilla-central. In this prototype, we note the script asynchronously whenever a new content process starts up and use mozJSSubScriptLoader to load Activity Stream scripts into the about:newtab window in RemotePageManager.

To ensure that the script gets registered in the ScriptPreloader cache, we need to ensure that the NoteScript function is called before the cache is flushed (i.e. before the "document-element-inserted" event). After experimenting some code, it seems like MainProcessSingleton would be a good place to do this since it falls into the window in which the ScriptPreloader is actively caching for requested scripts.

It seems like the window (in which is the ScriptPreloader is active) is sufficient for the asynchronous load to get the scripts into the cache. (There's a possibility that the script decoding will not make it in time before the "document-element-inserted" event, but I am not too sure how large of a concern is this.) If the script fails to make it into the cache in time, mozJSSubScriptLoader will fallback to decode and execute the script synchronously.

--

Here are some benchmarks based on the prototype:

The numbers correspond to the time taken from parsing the HTML to painting the data for topsites (i.e. when all of the data are ready).

Before:
First startup: 542ms (504-654ms)
New process: 349ms (334-395ms)
Existing process: 202ms (191-216ms)

With ScriptLoader (sync):
First startup: 541ms (515-568ms)
New process: 336ms (329-355ms)
Existing process: 174ms (164-215ms)

With mozJSSubScriptLoader (async):
First startup: 537ms (468-594ms)
New process: 340ms (305-375ms)
Existing process: 165ms (156-195ms)

It looks like we're getting about 18% decrease in overall time taken to load the about:newtab page in an existing process if we were to use mozJSSubScriptLoader. Based on the range, overall time seems to decrease as well (about 5 to 9%) for both first startup and new process (i.e. when a new tab gets opened in a new process).

I would love to get some thoughts on the prototype and the ideas above.

(On a side note, we should probably rename this bug to use the ScriptPreloader cache instead of the JS bytecode startup cache.)

Comment 23

[Nicolas B. Pierron [:nbp]]

(In reply to Jay Lim [:imjching] from comment #22)
> b) Use <script> tags to load those JS files. For this to work, we will need
> to modify EvaluateScript() in ScriptLoader so that it does not attempt to
> parse the script when it is already in the ScriptPreloader cache.

This is not the right place to address this issue.  If you want to use the ScriptLoader, I highly recommend to implement that as part of the Channel which is in charge of serving either the bytecode or the source.

The Alternate-Data API, which is used by Necko and by Service Workers, is made to abstract the the storage, and also avoiding one extra IPC round-trip for checking the ScriptPreloader cache.

If you were to add a corner case in EvaluateScript(), the channel would already have returned the source over IPC and it would be discarded immediately, thus wasted. The Alternate Data API avoid this issue by transferring either the Source or the Bytecode, but not both.

> We will
> also need to avoid opening the file again when we already have the script in
> the cache.

The script does not have to live in the cache, it depends what protocol is in charge of answering the channel request. The cache does not have to be the Necko cache. The cache could be the ScriptPreloader cache, which is waiting to be written to the disk if the channel remains open in the client.

> Also, I
> don't think we need to handle SRI for local resources, but since the SRI
> code is tightly coupled with ScriptLoader, this also suggests that
> ScriptLoader might not be a good place to use the ScriptPreloader cache.

The SRI is non argument, when there is no SRI hash we are saving 6 extra bytes (5 bytes for the empty SRI [1], and 1 byte of padding for the bytecode content) which are neglectable compared to the size of the bytecode.

If the concern is about saving the bytecode content, this is best addressed with the Alternate Data API, which let the content process save the bytes after the execution.

[1] https://searchfox.org/mozilla-central/search?q=symbol%3A_ZN7mozilla3dom20SRICheckDataVerifier22EmptyDataSummaryLengthEv&path=

Comment 24

[Jay Lim [:imjching]]

Once we have Activity Stream running in its own special content process, we can use mozJSSubScriptLoader that uses ScriptPreloader to load scripts.

I just filed bug 1470201 to move other about: pages to its own content process (and possibly use the ScriptPreloader as well for possible browser startup win).

Comment 25

[Jay Lim [:imjching]]

Attachment: Bug 1416066 - Set default ProcessType in ScriptPreloader to Uninitialized and update version of startup script cache file.

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

Comment 26

[Jay Lim [:imjching]]

Attachment: Bug 1416066 - Delay initialization of sProcessType for child processes in ScriptPreloader since remoteType in ContentChild is not ready yet.

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

Comment 27

[Jay Lim [:imjching]]

Attachment: Bug 1416066 - Add page-content-fully-loaded notification to process-content.js.

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

Comment 28

[Jay Lim [:imjching]]

Attachment: Bug 1416066 - Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes.

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

Comment 29

[Jay Lim [:imjching]]

Attachment: Bug 1416066 - Use XPConnect compilation scope for some non-cached local scripts with codebase principal when preload cache is enabled.

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

Comment 30

[Jay Lim [:imjching]]

Attachment: Bug 1416066 - Add preference observer for browser.newtabpage.enabled in aboutNewTabService.js.

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

Comment 31

[Jay Lim [:imjching]]

Attachment: Bug 1416066 - Add preference observer for browser.tabs.remote.separatePrivilegedContentProcess in aboutNewTabService.js.

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

Comment 32

[Jay Lim [:imjching]]

Attachment: Bug 1416066 - Render Activity Stream HTML files without scripts.

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

Comment 33

[Jay Lim [:imjching]]

Attachment: Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

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

Comment 34

[Code Review Bot [:reviewbot]]

Comment on attachment 8987204 [details]
Bug 1416066 - Add page-content-fully-loaded notification to process-content.js.

https://reviewboard.mozilla.org/r/252450/#review258958


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/process-content.js:72
(Diff revision 1)
> +          let loadListener = () => {
> +            subject.removeEventListener("load", loadListener);
> +            if (subject.document.location.pathname !== "blank") {
> +              Services.obs.notifyObservers(subject, "page-content-fully-loaded");
> +            }
> +          }

Error: Missing semicolon. [eslint: semi]

Comment 35

[Code Review Bot [:reviewbot]]

Comment on attachment 8987210 [details]
Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

https://reviewboard.mozilla.org/r/252462/#review258968


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/newtab/aboutNewTabService.js:176
(Diff revision 1)
> +          }
> +
> +          for (let script of scripts) {
> +            Services.scriptloader.loadSubScript(script, subject); // Synchronous call
> +          }
> +        }

Error: Missing semicolon. [eslint: semi]

Comment 36

[Jay Lim [:imjching]]

Comment on attachment 8987204 [details]
Bug 1416066 - Add page-content-fully-loaded notification to process-content.js.

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

Comment 37

[Jay Lim [:imjching]]

Comment on attachment 8987205 [details]
Bug 1416066 - Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes.

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

Comment 38

[Jay Lim [:imjching]]

Comment on attachment 8987206 [details]
Bug 1416066 - Use XPConnect compilation scope for some non-cached local scripts with codebase principal when preload cache is enabled.

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

Comment 39

[Jay Lim [:imjching]]

Comment on attachment 8987207 [details]
Bug 1416066 - Add preference observer for browser.newtabpage.enabled in aboutNewTabService.js.

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

Comment 40

[Jay Lim [:imjching]]

Comment on attachment 8987208 [details]
Bug 1416066 - Add preference observer for browser.tabs.remote.separatePrivilegedContentProcess in aboutNewTabService.js.

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

Comment 41

[Jay Lim [:imjching]]

Comment on attachment 8987209 [details]
Bug 1416066 - Render Activity Stream HTML files without scripts.

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

Comment 42

[Jay Lim [:imjching]]

Comment on attachment 8987210 [details]
Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

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

Comment 43

[Jay Lim [:imjching]]

Attachment: Bug 1416066 - Add generated prerendered Activity Stream HTML files without scripts ('npm run bundle').

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

Comment 44

[Jay Lim [:imjching]]

The patch above uses mozJSSubScriptLoader to load scripts into Activity Stream.

By default, "browser.tabs.remote.separatePrivilegedContentProcess" will be false. This means that Activity Stream will not be impacted unless that value is modified to true. When that happens, the noscripts version of the prerendered HTML files will be displayed to the user and scripts will be injected into the page. This approach has the advantage of using the ScriptPreloader to start the decoding early during process startup.

There are no thorough benchmarks for this yet, but based on some small ones, my guess would be that they would be similar to the ones above -- about 18% decrease in overall time taken to load the about:newtab page.

:k88hudson, :Mardak -- We will also need to backport some changes in this patch to the Activity Stream's GitHub repository. I will make a pull request early next week.

Comment 45

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8987206 [details]
Bug 1416066 - Use XPConnect compilation scope for some non-cached local scripts with codebase principal when preload cache is enabled.

https://reviewboard.mozilla.org/r/252454/#review259174

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:690
(Diff revision 2)
> -        || scheme.EqualsLiteral("blob");
> +        // Ignore caching if the script does not have system principal.
> +        // However, this rule is dropped if we know that we are in the
> +        // privileged content process.

Can a priviledge content process load unpriviledge content as well, such as third party scripts which are not statically bundled? Is this enforced anywhere that we should not?

Comment 46

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jay Lim [:imjching] from comment #43)
> Created attachment 8987235 [details]
> Bug 1416066 - Add generated prerendered Activity Stream HTML files without
> scripts ('npm run bundle').
> 
> Review commit: https://reviewboard.mozilla.org/r/252498/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/252498/

I highly suspect that the Activity Stream team are going to want to do this on their own during an export. Or is it necessary that all of these changes land at once?

Comment 47

[Jay Lim [:imjching]]

Comment on attachment 8987202 [details]
Bug 1416066 - Set default ProcessType in ScriptPreloader to Uninitialized and update version of startup script cache file.

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

Comment 48

[Jay Lim [:imjching]]

Comment on attachment 8987203 [details]
Bug 1416066 - Delay initialization of sProcessType for child processes in ScriptPreloader since remoteType in ContentChild is not ready yet.

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

Comment 49

[Jay Lim [:imjching]]

Comment on attachment 8987204 [details]
Bug 1416066 - Add page-content-fully-loaded notification to process-content.js.

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

Comment 50

[Jay Lim [:imjching]]

Comment on attachment 8987205 [details]
Bug 1416066 - Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes.

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

Comment 51

[Jay Lim [:imjching]]

Comment on attachment 8987206 [details]
Bug 1416066 - Use XPConnect compilation scope for some non-cached local scripts with codebase principal when preload cache is enabled.

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

Comment 52

[Jay Lim [:imjching]]

Comment on attachment 8987207 [details]
Bug 1416066 - Add preference observer for browser.newtabpage.enabled in aboutNewTabService.js.

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

Comment 53

[Jay Lim [:imjching]]

Comment on attachment 8987208 [details]
Bug 1416066 - Add preference observer for browser.tabs.remote.separatePrivilegedContentProcess in aboutNewTabService.js.

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

Comment 54

[Jay Lim [:imjching]]

Comment on attachment 8987209 [details]
Bug 1416066 - Render Activity Stream HTML files without scripts.

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

Comment 55

[Jay Lim [:imjching]]

Comment on attachment 8987210 [details]
Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

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

Comment 56

[Jay Lim [:imjching]]

Comment on attachment 8987235 [details]
Bug 1416066 - Add generated prerendered Activity Stream HTML files without scripts ('npm run bundle').

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

Comment 57

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4213

Comment 58

[Jay Lim [:imjching]]

Comment on attachment 8987210 [details]
Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252462/diff/3-4/

Comment 59

[Kris Maglione [:kmag]]

Comment on attachment 8987202 [details]
Bug 1416066 - Set default ProcessType in ScriptPreloader to Uninitialized and update version of startup script cache file.

https://reviewboard.mozilla.org/r/252446/#review259422

::: js/xpconnect/loader/ScriptPreloader.h:43
(Diff revision 2)
>  namespace loader {
>      class InputBuffer;
>      class ScriptCacheChild;
>  
>      enum class ProcessType : uint8_t {
> +        Uninitialized,

This requires bumping the version number in the magic number, since the type values all mean something different now. You should update script_cache.py, too, and add an assertion when decoding that we never decode this value.

Is this change really necessary, though?

Comment 60

[Kris Maglione [:kmag]]

Comment on attachment 8987203 [details]
Bug 1416066 - Delay initialization of sProcessType for child processes in ScriptPreloader since remoteType in ContentChild is not ready yet.

https://reviewboard.mozilla.org/r/252448/#review259424

::: commit-message-3e2c9:1
(Diff revision 2)
> +Bug 1416066 - Delay initialization of sProcessType in ScriptPreloader since remoteType in ContentChild is not ready yet.

Why is it not ready yet? We shouldn't instantiate the child preloader until after we get RecvSetRemoteType. The existing extension process code already depends on this.

I'd rather we assert that than delay instantiating the remote type.

Comment 61

[Andrew McCreight [:mccr8]]

I think Kris's review is sufficient for this. I'm not sure what this part of the code is trying to achieve.

Comment 62

[Kris Maglione [:kmag]]

Comment on attachment 8987204 [details]
Bug 1416066 - Add page-content-fully-loaded notification to process-content.js.

https://reviewboard.mozilla.org/r/252450/#review259430

::: toolkit/content/process-content.js:69
(Diff revision 3)
>      observe(subject, topic, data) {
>        switch (topic) {
> +        case "content-document-global-created": {
> +          let loadListener = () => {
> +            subject.removeEventListener("load", loadListener);
> +            if (subject.document.location.pathname !== "blank") {

I think you want `subject.location != "about:blank"`

We probably want to remove this observer once we fire it.

Also, please expand your commit message to explain what this is used for.

::: toolkit/content/process-content.js:73
(Diff revision 3)
> +            subject.removeEventListener("load", loadListener);
> +            if (subject.document.location.pathname !== "blank") {
> +              Services.obs.notifyObservers(subject, "page-content-fully-loaded");
> +            }
> +          };
> +          subject.addEventListener("load", loadListener);

Nit: Add `{once: true}` as a third param rather than removing the listener in the callback.

Comment 63

[Kris Maglione [:kmag]]

Comment on attachment 8987205 [details]
Bug 1416066 - Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes.

https://reviewboard.mozilla.org/r/252452/#review259432

::: js/xpconnect/loader/ScriptPreloader.cpp:345
(Diff revision 3)
> -    } else if (!strcmp(topic, DOC_ELEM_INSERTED_TOPIC)) {
> -        obs->RemoveObserver(this, DOC_ELEM_INSERTED_TOPIC);
> +    } else if (!strcmp(topic, DOC_ELEM_INSERTED_TOPIC) ||
> +               !strcmp(topic, PAGE_CONTENT_FULLY_LOADED_TOPIC)) {

I'd just store this in something like `mContentStartupFinishedTopic` when you create the observer and check that instead.

::: js/xpconnect/loader/ScriptPreloader.cpp:350
(Diff revision 3)
> -    } else if (!strcmp(topic, DOC_ELEM_INSERTED_TOPIC)) {
> -        obs->RemoveObserver(this, DOC_ELEM_INSERTED_TOPIC);
> +    } else if (!strcmp(topic, DOC_ELEM_INSERTED_TOPIC) ||
> +               !strcmp(topic, PAGE_CONTENT_FULLY_LOADED_TOPIC)) {
> +        obs->RemoveObserver(this, topic);
>  
>          MOZ_ASSERT(XRE_IsContentProcess());
> +        if (!strcmp(topic, PAGE_CONTENT_FULLY_LOADED_TOPIC)) {

You're going to wind up doing an extra strcmp here in debug builds, where the MOZ_ASSERTs aren't compiled.

Comment 64

[Kris Maglione [:kmag]]

Comment on attachment 8987206 [details]
Bug 1416066 - Use XPConnect compilation scope for some non-cached local scripts with codebase principal when preload cache is enabled.

https://reviewboard.mozilla.org/r/252454/#review259434

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:690
(Diff revision 3)
> -        || scheme.EqualsLiteral("blob");
> +        // Ignore caching if the script does not have system principal.
> +        // However, this rule is dropped if we know that we are in the
> +        // privileged content process.
> +        // Ref: https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/Script_security
> +        auto* cc = dom::ContentChild::GetSingleton();
> +        if (cc) {
> +            auto processType = ScriptPreloader::GetSingleton().GetChildProcessType(cc->GetRemoteType());
> +            if (processType != ProcessType::Privileged) {
> +                ignoreCache = !GetObjectPrincipal(targetObj)->GetIsSystemPrincipal();
> +            }
> +        } else {
> +            // We are not sure if we are in the privileged content process
> +            // since we cannot get the remoteType of the process.
> +            ignoreCache = !GetObjectPrincipal(targetObj)->GetIsSystemPrincipal();
> +        }

This isn't a good heuristic. We may still load remote pages in content process, and we definitely don't want to compile scripts for them.

We should probably just check IsSystemPrincipal() || IsCodebasePrincipal() && mCodebase->SchemeIs("resource")

Or whatever codebase URLs we use for activity stream... It may be about:home/about:newtab...

Comment 65

[[github robot]]

Commits pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/871916ab992315fcc0dac7b0c67b5224078b775d
Bug 1416066 - Render Activity Stream HTML files without scripts

Signed-off-by: Jay Lim <[e-mail]>

https://github.com/mozilla/activity-stream/commit/f3502adcd8d53e262314b21de1dd32b251e4f7f4
Merge pull request #4213 from imjching/bug1416066

Bug 1416066 - Render Activity Stream HTML files without scripts

Comment 66

[Jay Lim [:imjching]]

(In reply to Nicolas B. Pierron [:nbp] {backlog: 39} from comment #45)
> Comment on attachment 8987206 [details]
> Bug 1416066 - Drop system principal rule if loading scripts in privileged
> content processes.
> 
> https://reviewboard.mozilla.org/r/252454/#review259174
> 
> ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:690
> (Diff revision 2)
> > -        || scheme.EqualsLiteral("blob");
> > +        // Ignore caching if the script does not have system principal.
> > +        // However, this rule is dropped if we know that we are in the
> > +        // privileged content process.
> 
> Can a priviledge content process load unpriviledge content as well, such as
> third party scripts which are not statically bundled? Is this enforced
> anywhere that we should not?

Yes, a privileged content process can load unprivileged content at the moment. I don't think it is enforced anywhere yet. We might want to prevent access from loading and running scripts from the network in privileged content processes though.

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos / reviews) from comment #46)
> (In reply to Jay Lim [:imjching] from comment #43)
> > Created attachment 8987235 [details]
> > Bug 1416066 - Add generated prerendered Activity Stream HTML files without
> > scripts ('npm run bundle').
> > 
> > Review commit: https://reviewboard.mozilla.org/r/252498/diff/#index_header
> > See other reviews: https://reviewboard.mozilla.org/r/252498/
> 
> I highly suspect that the Activity Stream team are going to want to do this
> on their own during an export. Or is it necessary that all of these changes
> land at once?

Yes, all of these changes will have to land at once because we need to render the appropriate HTML (without scripts) when `browser.tabs.remote.separatePrivilegedContentProcess` is set to true. (See line 256 in https://reviewboard.mozilla.org/r/252462/diff/3#index_header.) That patch should not affect Activity Stream because we are only adding new files.

However, I just thought of a different approach. I dropped two patches here and moved them to Activity Stream's GitHub repo yesterday: https://github.com/mozilla/activity-stream/pull/4213. Before landing this patch, we will have to merge that and export those changes because this patch depends on the generated Activity Stream HTML files without scripts.

Comment 67

[Dan Mosedale (:dmosedale, :dmose)]

If you wanted to go with your original approach, would it be good enough to simply not announce the pref and leave it set to the default of false until the second patch lands?

Comment 68

[Jay Lim [:imjching]]

Comment on attachment 8987202 [details]
Bug 1416066 - Set default ProcessType in ScriptPreloader to Uninitialized and update version of startup script cache file.

https://reviewboard.mozilla.org/r/252446/#review259664

::: js/xpconnect/loader/ScriptPreloader.h:43
(Diff revision 2)
>  namespace loader {
>      class InputBuffer;
>      class ScriptCacheChild;
>  
>      enum class ProcessType : uint8_t {
> +        Uninitialized,

I made this change because in the other patch, I delayed the initialization of `sProcessType` in ScriptPreloader. If this change wasn't made, then the default value would be `ProcessType::Parent`, which might be an issue.

Comment 69

[Jay Lim [:imjching]]

Comment on attachment 8987203 [details]
Bug 1416066 - Delay initialization of sProcessType for child processes in ScriptPreloader since remoteType in ContentChild is not ready yet.

https://reviewboard.mozilla.org/r/252448/#review259684

::: commit-message-3e2c9:1
(Diff revision 2)
> +Bug 1416066 - Delay initialization of sProcessType in ScriptPreloader since remoteType in ContentChild is not ready yet.

Hm. Interesting.

It does not seem to be that case when I attempted to log `NS_ConvertUTF16toUTF8(dom::ContentChild::GetSingleton()->GetRemoteType()).get())` in both the ScriptPreloader constructor and the child's `InitCache()`. The former shows an empty string, whereas the latter returns a value (e.g. web, privileged, extension).

It seems like the ScriptPreloader for the child was created before `RecvSetRemoteType` and hence the empty string. If we were to rely on the existing code, `sProcessType` for the child will always be `web` and we won't be able to add an observer for a different topic when it comes to `sProcessType == ProcessType::Privileged`.

Comment 70

[Jay Lim [:imjching]]

Comment on attachment 8987204 [details]
Bug 1416066 - Add page-content-fully-loaded notification to process-content.js.

https://reviewboard.mozilla.org/r/252450/#review259686

::: toolkit/content/process-content.js:69
(Diff revision 3)
>      observe(subject, topic, data) {
>        switch (topic) {
> +        case "content-document-global-created": {
> +          let loadListener = () => {
> +            subject.removeEventListener("load", loadListener);
> +            if (subject.document.location.pathname !== "blank") {

`process-content.js` is executed once everytime a new process is created (https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/toolkit/components/processsingleton/MainProcessSingleton.js#73).

We are removing the observer for `content-document-global-created` in the `uninit()` function above and this function is called when we receive the `xpcom-shutdown` event.

Comment 71

[Jay Lim [:imjching]]

(In reply to Dan Mosedale (:dmose) from comment #67)
> If you wanted to go with your original approach, would it be good enough to
> simply not announce the pref and leave it set to the default of false until
> the second patch lands?

(I suppose you mean the second patch here refers to the export?)

Yes, that's right, but either way, the "browser.tabs.remote.separatePrivilegedContentProcess" preference is set to the default value of false in Bug 1469072. I will file a new bug to modify that default value to true.

In my original approach, I generated the "noscripts" version of Activity Stream. By doing that, we will need to backport those changes into the Activity Stream GitHub repo, and when that gets exported, the prerendered files (which includes the "noscripts" version) will be regenerated again. The original approach makes this patch and the export independent, whereas in the new approach, this patch is blocked by that export.

Comment 72

[Nicolas B. Pierron [:nbp]]

(In reply to Jay Lim [:imjching] from comment #66)
> (In reply to Nicolas B. Pierron [:nbp] {backlog: 39} from comment #45)
> > ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:690
> > (Diff revision 2)
> > > -        || scheme.EqualsLiteral("blob");
> > > +        // Ignore caching if the script does not have system principal.
> > > +        // However, this rule is dropped if we know that we are in the
> > > +        // privileged content process.
> > 
> > Can a priviledge content process load unpriviledge content as well, such as
> > third party scripts which are not statically bundled? Is this enforced
> > anywhere that we should not?
> 
> Yes, a privileged content process can load unprivileged content at the
> moment. I don't think it is enforced anywhere yet. We might want to prevent
> access from loading and running scripts from the network in privileged
> content processes though.

We should prevent access by either having priviledged inputs or unpriviledged inputs but *not both*.
Having both breaks Firefox security model. (similar to Bug 1426353)

Comment 73

[Kate Hudson :k88hudson]

Comment on attachment 8987207 [details]
Bug 1416066 - Add preference observer for browser.newtabpage.enabled in aboutNewTabService.js.

https://reviewboard.mozilla.org/r/252456/#review259808

This part looks good!

Comment 74

[Kate Hudson :k88hudson]

Comment on attachment 8987208 [details]
Bug 1416066 - Add preference observer for browser.tabs.remote.separatePrivilegedContentProcess in aboutNewTabService.js.

https://reviewboard.mozilla.org/r/252458/#review259810

Comment 75

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8987204 [details]
Bug 1416066 - Add page-content-fully-loaded notification to process-content.js.

https://reviewboard.mozilla.org/r/252450/#review260100

::: toolkit/content/process-content.js:66
(Diff revision 3)
>        });
>      },
>  
>      observe(subject, topic, data) {
>        switch (topic) {
> +        case "content-document-global-created": {

Instead of adding an observer for when the global is created, and then listening for a load event, we might be able to find a later notification that is late enough for our purposes.

For example, there is "content-document-loaded", which is fired here: https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/layout/base/nsDocumentViewer.cpp#1154

which, now that I think about it, might be analagous to page-content-fully-loaded.

So instead of adding a new notification called page-content-fully-loaded, can we have ScriptPreloader listen for content-document-loaded?

::: toolkit/content/process-content.js:66
(Diff revision 3)
>        });
>      },
>  
>      observe(subject, topic, data) {
>        switch (topic) {
> +        case "content-document-global-created": {

Instead of adding an observer for when the global is created, and then listening for a load event, we might be able to find a later notification that is late enough for our purposes.

For example, there is "content-document-loaded", which is fired here: https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/layout/base/nsDocumentViewer.cpp#1154

which, now that I think about it, might be analagous to page-content-fully-loaded.

So instead of adding a new notification called page-content-fully-loaded, can we have ScriptPreloader listen for content-document-loaded?

If we go that route, we'll likely need to update ScriptPreloader to ignore content-fully-loaded if the document location is about:blank.

Comment 76

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8987205 [details]
Bug 1416066 - Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes.

https://reviewboard.mozilla.org/r/252452/#review259432

> You're going to wind up doing an extra strcmp here in debug builds, where the MOZ_ASSERTs aren't compiled.

I agree - perhaps this should be wrapped in an #ifdef DEBUG.

Comment 77

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8987205 [details]
Bug 1416066 - Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes.

https://reviewboard.mozilla.org/r/252452/#review260102

::: js/xpconnect/loader/ScriptPreloader.cpp:37
(Diff revision 3)
>  #include "nsXULAppAPI.h"
>  #include "xpcpublic.h"
>  
>  #define STARTUP_COMPLETE_TOPIC "browser-delayed-startup-finished"
>  #define DOC_ELEM_INSERTED_TOPIC "document-element-inserted"
> +#define PAGE_CONTENT_FULLY_LOADED_TOPIC "page-content-fully-loaded"

So, if the idea in my previous review holds, this should be content-document-loaded.

::: js/xpconnect/loader/ScriptPreloader.cpp:467
(Diff revision 3)
> +        // Since we know that the process is privileged, it will always be safe
> +        // and we can increase the window in which the ScriptPreloader is active.

"it will always be safe" is maybe a bit ambiguous. Perhaps we should say,

"Since we control all of the documents loaded in the privileged content process, we can increase the window of active time for the preloader to include the scripts that are loaded until the first document finishes."

something along those lines, anyhow. Also, we should probably find a way of ensuring that the privileged content process never loads untrusted script. We should at the very least file a bug on that and look into what can be done.

Comment 78

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8987210 [details]
Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

https://reviewboard.mozilla.org/r/252462/#review260110

::: browser/components/newtab/aboutNewTabService.js:57
(Diff revision 4)
>    this.initialized = true;
>  
>    if (IS_MAIN_PROCESS) {
>      AboutNewTab.init();
> +  } else {
> +    Services.obs.addObserver(this, TOPIC_CONTENT_DOCUMENT_CREATED);

When is this observer removed?

::: browser/components/newtab/aboutNewTabService.js:130
(Diff revision 4)
>            this._activityStreamDebug = Services.prefs.getBoolPref(PREF_ACTIVITY_STREAM_DEBUG, false);
>            this.updatePrerenderedPath();
>            this.notifyChange();
>          }
>          break;
> +      case TOPIC_CONTENT_DOCUMENT_CREATED:

Maybe instead of waiting for TOPIC_CONTENT_DOCUMENT_CREATED, you could wait for content-document-interactive, which is fired just before the DOMContentLoaded event:

https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/dom/base/nsDocument.cpp#4958-4962

::: browser/components/newtab/aboutNewTabService.js:139
(Diff revision 4)
> +          // Do not inject scripts if we are not using separate privileged
> +          // content processes.
> +          if (!this._privilegedContentProcess) {
> +            return;
> +          }
> +

At this point, we know that the separate privileged content process is enabled, and we're going to see if we're supposed to inject script.

Perhaps at this point, we can bail out earlier if we know that our processType isn't the privileged content process.

And if we _are_ loading content in the privileged content process, then Activity Stream must be enabled, no?

I just feel like a lot of these things being checked here can be assumed (like, if we got here, and we're in the privileged content process, then we must be loading an about: page) Perhaps we should assert those things.

(For example, I don't think it should be possible to get here in the privileged process if the URL for about:newtab has been overridden. At least, it certainly shouldn't be).

Comment 79

[Jay Lim [:imjching]]

(In reply to Nicolas B. Pierron [:nbp] {backlog: 39} from comment #72)
> (In reply to Jay Lim [:imjching] from comment #66)
> > (In reply to Nicolas B. Pierron [:nbp] {backlog: 39} from comment #45)
> > > ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:690
> > > (Diff revision 2)
> > > > -        || scheme.EqualsLiteral("blob");
> > > > +        // Ignore caching if the script does not have system principal.
> > > > +        // However, this rule is dropped if we know that we are in the
> > > > +        // privileged content process.
> > > 
> > > Can a priviledge content process load unpriviledge content as well, such as
> > > third party scripts which are not statically bundled? Is this enforced
> > > anywhere that we should not?
> > 
> > Yes, a privileged content process can load unprivileged content at the
> > moment. I don't think it is enforced anywhere yet. We might want to prevent
> > access from loading and running scripts from the network in privileged
> > content processes though.
> 
> We should prevent access by either having priviledged inputs or
> unpriviledged inputs but *not both*.
> Having both breaks Firefox security model. (similar to Bug 1426353)

When you say "inputs" here, are you referring to pages that will be loaded in the content process?

Also, could you add :mconley and I to the CC list on the bug? I do not have access to that bug at the moment.

Comment 80

[Kris Maglione [:kmag]]

(In reply to Nicolas B. Pierron [:nbp] {backlog: 39} from comment #72)
> (In reply to Jay Lim [:imjching] from comment #66)
> > (In reply to Nicolas B. Pierron [:nbp] {backlog: 39} from comment #45)
> > > Can a priviledge content process load unpriviledge content as well, such as
> > > third party scripts which are not statically bundled? Is this enforced
> > > anywhere that we should not?
> > 
> > Yes, a privileged content process can load unprivileged content at the
> > moment. I don't think it is enforced anywhere yet. We might want to prevent
> > access from loading and running scripts from the network in privileged
> > content processes though.
> 
> We should prevent access by either having priviledged inputs or
> unpriviledged inputs but *not both*.
> Having both breaks Firefox security model. (similar to Bug 1426353)

We should. But at the moment, the privileged content we load into the process is responsible for making sure it doesn't load anything unprivileged before we snapshot its loaded scripts. I'd rather we had a platform way to enforce this, but we have the same issue in the parent process where we similarly have no real way to enforce it, so I don't think this is a blocker.

I do think it's something we should make a priority of fixing, though.

Comment 81

[Jay Lim [:imjching]]

Comment on attachment 8987210 [details]
Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

https://reviewboard.mozilla.org/r/252462/#review260170

::: browser/components/newtab/aboutNewTabService.js:57
(Diff revision 4)
>    this.initialized = true;
>  
>    if (IS_MAIN_PROCESS) {
>      AboutNewTab.init();
> +  } else {
> +    Services.obs.addObserver(this, TOPIC_CONTENT_DOCUMENT_CREATED);

It should be removed when the "quit-application-granted" event is received.

I missed that in this patch, but I have it in the updated patch.

::: browser/components/newtab/aboutNewTabService.js:130
(Diff revision 4)
>            this._activityStreamDebug = Services.prefs.getBoolPref(PREF_ACTIVITY_STREAM_DEBUG, false);
>            this.updatePrerenderedPath();
>            this.notifyChange();
>          }
>          break;
> +      case TOPIC_CONTENT_DOCUMENT_CREATED:

That should work too. Out of curiosity, are there any benefits to waiting for "content-document-interactive" instead of "content-document-global-created"? (I'm assuming that "content-document-interactive" should give us the window object as well, but I've not verified that yet.)

Comment 82

[Nicolas B. Pierron [:nbp]]

(In reply to Jay Lim [:imjching] from comment #79)
> (In reply to Nicolas B. Pierron [:nbp] {backlog: 39} from comment #72)
> > We should prevent access by either having priviledged inputs or
> > unpriviledged inputs but *not both*.
> > Having both breaks Firefox security model. (similar to Bug 1426353)
> 
> When you say "inputs" here, are you referring to pages that will be loaded
> in the content process?

I am referring to pages and scripts.
In general, anything that we are loading, caching and reloading from a cache.

> Also, could you add :mconley and I to the CC list on the bug? I do not have
> access to that bug at the moment.

Now you should both have access to it.

Comment 83

[Jay Lim [:imjching]]

Comment on attachment 8987206 [details]
Bug 1416066 - Use XPConnect compilation scope for some non-cached local scripts with codebase principal when preload cache is enabled.

https://reviewboard.mozilla.org/r/252454/#review259770

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:690
(Diff revision 3)
> -        || scheme.EqualsLiteral("blob");
> +        // Ignore caching if the script does not have system principal.
> +        // However, this rule is dropped if we know that we are in the
> +        // privileged content process.
> +        // Ref: https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/Script_security
> +        auto* cc = dom::ContentChild::GetSingleton();
> +        if (cc) {
> +            auto processType = ScriptPreloader::GetSingleton().GetChildProcessType(cc->GetRemoteType());
> +            if (processType != ProcessType::Privileged) {
> +                ignoreCache = !GetObjectPrincipal(targetObj)->GetIsSystemPrincipal();
> +            }
> +        } else {
> +            // We are not sure if we are in the privileged content process
> +            // since we cannot get the remoteType of the process.
> +            ignoreCache = !GetObjectPrincipal(targetObj)->GetIsSystemPrincipal();
> +        }

When you say content process here, are you referring to content processes in general, or would that be the privileged content process? If you're referring to the latter, I don't think we plan to load any remote pages in it, though it's not enforced anywhere yet.

But yes, I agree that it's not a good heuristic. Checking the codebase principal and the scheme sounds like a better approach.

Comment 84

[Jay Lim [:imjching]]

Attachment: Bug 1416066 - Update script_cache.py to support new version of startup script cache file.

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

Comment 85

[Jay Lim [:imjching]]

Attachment: Bug 1416066 - Use XPConnect compilation scope for non-cached local scripts with codebase principal when preload cache is enabled.

When we use the preload cache for non-cached local scripts in loadSubScript,
we will keep the global that the script was compiled for alive, resulting in a
leak.

We will compile chrome:// and resource:// scripts with codebase principal in
the XPConnect compilation scope when using mozJSSubScriptLoader to load scripts
synchronously. When the script is evaluated, it will be cloned into the target
scope to be executed. By compiling the script in a different scope, we can avoid
keeping the global that the script was compiled for originally alive.

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

Comment 86

[Jay Lim [:imjching]]

Comment on attachment 8987202 [details]
Bug 1416066 - Set default ProcessType in ScriptPreloader to Uninitialized and update version of startup script cache file.

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

Comment 87

[Jay Lim [:imjching]]

Comment on attachment 8987203 [details]
Bug 1416066 - Delay initialization of sProcessType for child processes in ScriptPreloader since remoteType in ContentChild is not ready yet.

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

Comment 88

[Jay Lim [:imjching]]

Comment on attachment 8987205 [details]
Bug 1416066 - Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252452/diff/3-4/

Comment 89

[Jay Lim [:imjching]]

Comment on attachment 8987206 [details]
Bug 1416066 - Use XPConnect compilation scope for some non-cached local scripts with codebase principal when preload cache is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252454/diff/3-4/

Comment 90

[Jay Lim [:imjching]]

Comment on attachment 8987208 [details]
Bug 1416066 - Add preference observer for browser.tabs.remote.separatePrivilegedContentProcess in aboutNewTabService.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252458/diff/3-4/

Comment 91

[Jay Lim [:imjching]]

Comment on attachment 8987210 [details]
Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252462/diff/4-5/

Comment 92

[Jay Lim [:imjching]]

I have updated the patches to address all the comments.

I fixed a memory leak issue when using mozJSSubScriptLoader with the ScriptPreloader to load scripts into the window of Activity Stream. We will now compile the non-cached chrome:// and resource:// scripts with codebase principal in the XPConnect compilation scope, preventing the window of Activity Stream from being kept alive.

Pushed to TryServer: https://treeherder.mozilla.org/#/jobs?repo=try&revision=21da1567bf3a6792f9c41f136659f4d5070dd01e

Also, the pull request in the Activity Stream GitHub repository was merged a couple of days ago: https://github.com/mozilla/activity-stream/pull/4213. We will need to do an export before landing this one. That pull request attempts to render Activity Stream HTML files without scripts.

Comment 93

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8987205 [details]
Bug 1416066 - Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes.

https://reviewboard.mozilla.org/r/252452/#review260732

Thanks, Jay! This looks okay to me, but let's get kmag's take too.

Comment 94

[Kris Maglione [:kmag]]

Comment on attachment 8987206 [details]
Bug 1416066 - Use XPConnect compilation scope for some non-cached local scripts with codebase principal when preload cache is enabled.

https://reviewboard.mozilla.org/r/252454/#review260794

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:688
(Diff revision 4)
> +    bool isCodebaseFile = GetObjectPrincipal(targetObj)->GetIsCodebasePrincipal()
> +        && (scheme.EqualsLiteral("chrome") || scheme.EqualsLiteral("resource"));

This isn't quite what I meant. The scheme of the script we're loading isn't really what we care about here. That will often be "resource:" or "chrome:" even in cases where we're loading into content compartments where we probably don't want caching.

The scheme of the codebase principal is what we care about.

Which means we need something like:

    auto* principal = BasePrincipal::Cast(GetObjectPrincipal(targetObj));
    bool isSystem = principal->Is<SystemPrincipal>();
    if (!isSystem && principal->Is<ContentPrincipal>()) {
        nsAutoCString scheme;
        principal->As<ContentPrincipal>()->mCodebase->GetScheme(scheme);

        isSystem = scheme.EqualsLiteral("resource") || scheme.EqualsLiteral("chrome");
    }

Comment 95

[Kris Maglione [:kmag]]

Comment on attachment 8988734 [details]
Bug 1416066 - Use XPConnect compilation scope for non-cached local scripts with codebase principal when preload cache is enabled.

https://reviewboard.mozilla.org/r/253944/#review260798

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:558
(Diff revision 1)
>  
>      nsCString buf;
>      rv = NS_ReadInputStreamToString(instream, buf, len);
>      NS_ENSURE_SUCCESS(rv, false);
>  
> +    AutoJSAPI jsapi;

No need for all of this. You just want something like:

    Maybe<JSAutoRealm> ar;
    if (useCompilationScope) {
      ar.emplace(cx, xpc::CompilationScope());
    }

We should also change PrepareScript to take something like a `bool wantGlobalScript` arg rather than a target JS Object, since that object will now sometimes be in a different compartment than the JS context, which will cause crashes if we use it in the wrong way.

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:746
(Diff revision 1)
>          // |back there.
>          cache = nullptr;
>      } else if (!ReadScript(uri, cx, targetObj, options.charset,
>                          static_cast<const char*>(uriStr.get()), serv,
> -                        options.wantReturnValue, &script)) {
> +                        options.wantReturnValue,
> +                        isCodebaseFile && !options.wantReturnValue, &script)) {

I don't think `wantReturnValue` should have an effect on this.

Comment 96

[Kris Maglione [:kmag]]

Jay, can you run Firefox in a debugger and try to find out where the preloader is getting initialized before the process type is set?

I'd really rather avoid that instead of delaying initialization of that value if at all possible.

Comment 97

[Jay Lim [:imjching]]

(In reply to Kris Maglione [:kmag] from comment #96)
> Jay, can you run Firefox in a debugger and try to find out where the
> preloader is getting initialized before the process type is set?
> 
> I'd really rather avoid that instead of delaying initialization of that
> value if at all possible.

The preloader is getting initialized in NS_InitXPCOM2 (https://searchfox.org/mozilla-central/source/xpcom/build/XPCOMInit.cpp#720). This is called by Init() in XRE_InitChildProcess (https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/toolkit/xre/nsEmbedFunctions.cpp#696-698),

The remoteType of the content process is set when we run the UI event loop on the main thread. (https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/toolkit/xre/nsEmbedFunctions.cpp#721-722), and this happens after the Init() function above.

After discussing with :mconley about this, we're not too sure if it's a good idea to initialize the ScriptPreloader just after setting the remoteType: https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/dom/ipc/ContentChild.cpp#2764. If we were to do this, we will be delaying the reading of cache file from disk as well.

Alternatively, before starting the event loop, we can process the setting of remote type separately by blocking and waiting for that IPC message. Once we have the remote type of the content process, we will initialize the ScriptPreloader and start the UI event loop to process more events. I'm not too sure if this is possible though.

Comment 98

[Jay Lim [:imjching]]

Attachment: Bug 1416066 - Enable caching for scripts with codebase URLs of about:home, about:newtab, and about:welcome.

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

Comment 99

[Jay Lim [:imjching]]

Attachment: Bug 1416066 - Add a new flag to nsIAboutModule to load URIs in privileged content processes if feature is enabled.

We will apply the URI_CAN_LOAD_IN_PRIVILEGED_CHILD flag to Activity Stream about: pages instead of hardcoding the URLs in a Set.

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

Comment 100

[Jay Lim [:imjching]]

Comment on attachment 8987202 [details]
Bug 1416066 - Set default ProcessType in ScriptPreloader to Uninitialized and update version of startup script cache file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252446/diff/3-4/

Comment 101

[Jay Lim [:imjching]]

Comment on attachment 8988733 [details]
Bug 1416066 - Update script_cache.py to support new version of startup script cache file.

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

Comment 102

[Jay Lim [:imjching]]

Comment on attachment 8987203 [details]
Bug 1416066 - Delay initialization of sProcessType for child processes in ScriptPreloader since remoteType in ContentChild is not ready yet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252448/diff/3-4/

Comment 103

[Jay Lim [:imjching]]

Comment on attachment 8987205 [details]
Bug 1416066 - Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252452/diff/4-5/

Comment 104

[Jay Lim [:imjching]]

Comment on attachment 8987206 [details]
Bug 1416066 - Use XPConnect compilation scope for some non-cached local scripts with codebase principal when preload cache is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252454/diff/4-5/

Comment 105

[Jay Lim [:imjching]]

Comment on attachment 8987208 [details]
Bug 1416066 - Add preference observer for browser.tabs.remote.separatePrivilegedContentProcess in aboutNewTabService.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252458/diff/4-5/

Comment 106

[Jay Lim [:imjching]]

Comment on attachment 8987210 [details]
Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252462/diff/5-6/

Comment 107

[Kate Hudson :k88hudson]

Comment on attachment 8987210 [details]
Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

https://reviewboard.mozilla.org/r/252462/#review259812

::: browser/components/newtab/aboutNewTabService.js:141
(Diff revision 4)
> +          if (!this._privilegedContentProcess) {
> +            return;
> +          }
> +
> +          // Do not inject scripts if we do not need them.
> +          if (subject.document.location.pathname == "newtab") {

This should be ===

Comment 108

[Jay Lim [:imjching]]

(In reply to Kate Hudson :k88hudson from comment #107)
> Comment on attachment 8987210 [details]
> Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts
> when DOMContentLoaded is fired.
> 
> https://reviewboard.mozilla.org/r/252462/#review259812
> 
> ::: browser/components/newtab/aboutNewTabService.js:141
> (Diff revision 4)
> > +          if (!this._privilegedContentProcess) {
> > +            return;
> > +          }
> > +
> > +          // Do not inject scripts if we do not need them.
> > +          if (subject.document.location.pathname == "newtab") {
> 
> This should be ===

That is an old revision. Revision 6 does not check that anymore.

Comment 109

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8989780 [details]
Bug 1416066 - Add a new flag to nsIAboutModule to load URIs in privileged content processes if feature is enabled.

https://reviewboard.mozilla.org/r/254640/#review261946

Yeah, I like this much better, thanks.

Comment 110

[Ed Lee :Mardak]

Comment on attachment 8987210 [details]
Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

https://reviewboard.mozilla.org/r/252462/#review262008

::: browser/components/newtab/aboutNewTabService.js:149
(Diff revision 6)
> +        }
> +
> +        const onLoaded = () => {
> +          const debugString = this._activityStreamDebug ? "-dev" : "";
> +          const scripts = [
> +            "chrome://browser/content/contentSearchUI.js",

Just a heads up that bug 1347204 is in the process of landing, and it adds "chrome://browser/content/contentTheme.js"

I can't think of a better way to keep these lists of scripts up to date for aboutNewTabService and render-activity-stream-html although once the noscripts version is the only version, the list would only need to exist in this file?

But even then, it seems somewhat unfortunate that this would need to live outside of the activity-stream repository…

https://github.com/mozilla/activity-stream/pull/4210/files#diff-92b55e4a266df72d234dd8551ab6ca10R181

Comment 111

[Kris Maglione [:kmag]]

Comment on attachment 8987205 [details]
Bug 1416066 - Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes.

https://reviewboard.mozilla.org/r/252452/#review262076

::: js/xpconnect/loader/ScriptPreloader.cpp:350
(Diff revision 5)
>  
>          if (mChildCache) {
>              Unused << NS_NewNamedThread("SaveScripts",
>                                          getter_AddRefs(mSaveThread), this);
>          }
> -    } else if (!strcmp(topic, DOC_ELEM_INSERTED_TOPIC)) {
> +    } else if (mContentStartupFinishedTopic.Length() > 0 &&

The Length() check is not necessary.

::: js/xpconnect/loader/ScriptPreloader.cpp:502
(Diff revision 5)
> +    if (sProcessType == ProcessType::Privileged) {
> +        // Since we control all of the documents loaded in the privileged
> +        // content process, we can increase the window of active time for the
> +        // ScriptPreloader to include the scripts that are loaded until the
> +        // first document finishes loading.
> +        mContentStartupFinishedTopic = CONTENT_DOCUMENT_LOADED_TOPIC;

`.AssignLiteral(CONTENT_...)`

Same for below.

Comment 112

[Kris Maglione [:kmag]]

Comment on attachment 8987206 [details]
Bug 1416066 - Use XPConnect compilation scope for some non-cached local scripts with codebase principal when preload cache is enabled.

https://reviewboard.mozilla.org/r/252454/#review262080

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:752
(Diff revision 5)
>          // |script| came from the cache, so don't bother writing it
>          // |back there.
>          cache = nullptr;
>      } else if (!ReadScript(uri, cx, targetObj, options.charset,
>                          static_cast<const char*>(uriStr.get()), serv,
> -                        options.wantReturnValue, &script)) {
> +                        options.wantReturnValue, isSystem, &script)) {

We only want to do this for non-system principals that we're going to be caching. I.e., about:home, about:newtab, about:welcome. We should have a separate variable for that, and it should have a name like `useCompilationScope`.

r=me with that

Comment 113

[Kris Maglione [:kmag]]

Comment on attachment 8989779 [details]
Bug 1416066 - Enable caching for scripts with codebase URLs of about:home, about:newtab, and about:welcome.

https://reviewboard.mozilla.org/r/254638/#review262082

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:696
(Diff revision 1)
> -        || !GetObjectPrincipal(targetObj)->GetIsSystemPrincipal()
> -        || scheme.EqualsLiteral("blob");
> +    bool isSystem = principal->Is<SystemPrincipal>();
> +    if (!isSystem && principal->Is<ContentPrincipal>()) {
> +        auto* content = principal->As<ContentPrincipal>();
> +        nsAutoCString scheme, filePath;
> +        content->mCodebase->GetScheme(scheme);
> +        content->mCodebase->GetFilePath(filePath);

We should probably only call `GetFilePath` if the scheme is "about". It can be more expensive than you might expect.

Comment 114

[Kris Maglione [:kmag]]

Comment on attachment 8987202 [details]
Bug 1416066 - Set default ProcessType in ScriptPreloader to Uninitialized and update version of startup script cache file.

https://reviewboard.mozilla.org/r/252446/#review262084

Comment 115

[Kris Maglione [:kmag]]

Comment on attachment 8987203 [details]
Bug 1416066 - Delay initialization of sProcessType for child processes in ScriptPreloader since remoteType in ContentChild is not ready yet.

https://reviewboard.mozilla.org/r/252448/#review262086

r=me with these changes

::: js/xpconnect/loader/ScriptPreloader.cpp
(Diff revision 4)
> -    if (XRE_IsParentProcess()) {
> -        sProcessType = ProcessType::Parent;

Please keep this here. It's guaranteed to be valid at this point. The content process version can still move.

::: js/xpconnect/loader/ScriptPreloader.cpp
(Diff revision 4)
> -    if (XRE_IsParentProcess()) {
> -        // In the parent process, we want to freeze the script cache as soon
> -        // as idle tasks for the first browser window have completed.
> -        obs->AddObserver(this, STARTUP_COMPLETE_TOPIC, false);
> -        obs->AddObserver(this, CACHE_WRITE_TOPIC, false);
> -    } else {
> -        // In the child process, we need to freeze the script cache before any
> -        // untrusted code has been executed. The insertion of the first DOM
> -        // document element may sometimes be earlier than is ideal, but at
> -        // least it should always be safe.
> -        obs->AddObserver(this, DOC_ELEM_INSERTED_TOPIC, false);
> -    }

Please keep these here.

::: js/xpconnect/loader/ScriptPreloader.cpp:341
(Diff revision 4)
>          MOZ_ASSERT(XRE_IsParentProcess());
>  
>          mStartupFinished = true;
>      } else if (!strcmp(topic, CACHE_WRITE_TOPIC)) {
>          obs->RemoveObserver(this, CACHE_WRITE_TOPIC);
> +

This seems unrelated.

Comment 116

[Kris Maglione [:kmag]]

Comment on attachment 8988733 [details]
Bug 1416066 - Update script_cache.py to support new version of startup script cache file.

https://reviewboard.mozilla.org/r/253942/#review262088

Thanks

Comment 117

[Ed Lee :Mardak]

Comment on attachment 8987210 [details]
Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

https://reviewboard.mozilla.org/r/252462/#review262304

::: browser/components/newtab/aboutNewTabService.js:149
(Diff revision 6)
> +        }
> +
> +        const onLoaded = () => {
> +          const debugString = this._activityStreamDebug ? "-dev" : "";
> +          const scripts = [
> +            "chrome://browser/content/contentSearchUI.js",

The other bug has made it to m-c, so the contentTheme script should be added now.

Comment 118

[Jay Lim [:imjching]]

Comment on attachment 8987203 [details]
Bug 1416066 - Delay initialization of sProcessType for child processes in ScriptPreloader since remoteType in ContentChild is not ready yet.

https://reviewboard.mozilla.org/r/252448/#review262880

::: js/xpconnect/loader/ScriptPreloader.cpp
(Diff revision 4)
> -    if (XRE_IsParentProcess()) {
> -        // In the parent process, we want to freeze the script cache as soon
> -        // as idle tasks for the first browser window have completed.
> -        obs->AddObserver(this, STARTUP_COMPLETE_TOPIC, false);
> -        obs->AddObserver(this, CACHE_WRITE_TOPIC, false);
> -    } else {
> -        // In the child process, we need to freeze the script cache before any
> -        // untrusted code has been executed. The insertion of the first DOM
> -        // document element may sometimes be earlier than is ideal, but at
> -        // least it should always be safe.
> -        obs->AddObserver(this, DOC_ELEM_INSERTED_TOPIC, false);
> -    }

I plan to only keep the first `if` block here. That `if` block adds the observers for the parent process. In the next commit, we will need to use `sProcessType` to determine the value of `mContentStartupFinishedTopic`. I feel that the change is more relevant in this commit.

::: js/xpconnect/loader/ScriptPreloader.cpp:341
(Diff revision 4)
>          MOZ_ASSERT(XRE_IsParentProcess());
>  
>          mStartupFinished = true;
>      } else if (!strcmp(topic, CACHE_WRITE_TOPIC)) {
>          obs->RemoveObserver(this, CACHE_WRITE_TOPIC);
> +

This was here because I rebased by changes against central.

Comment 119

[Jay Lim [:imjching]]

Comment on attachment 8987205 [details]
Bug 1416066 - Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes.

https://reviewboard.mozilla.org/r/252452/#review262890

::: js/xpconnect/loader/ScriptPreloader.cpp:350
(Diff revision 5)
>  
>          if (mChildCache) {
>              Unused << NS_NewNamedThread("SaveScripts",
>                                          getter_AddRefs(mSaveThread), this);
>          }
> -    } else if (!strcmp(topic, DOC_ELEM_INSERTED_TOPIC)) {
> +    } else if (mContentStartupFinishedTopic.Length() > 0 &&

Why? Can we assume that `topic` will never be an empty string? `mContentStartupFinishedTopic` will not be initialized in the parent process.

Would asserting that it is a content process be better here?

Comment 120

[Jay Lim [:imjching]]

Comment on attachment 8987210 [details]
Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

https://reviewboard.mozilla.org/r/252462/#review262922

::: browser/components/newtab/aboutNewTabService.js:149
(Diff revision 6)
> +        }
> +
> +        const onLoaded = () => {
> +          const debugString = this._activityStreamDebug ? "-dev" : "";
> +          const scripts = [
> +            "chrome://browser/content/contentSearchUI.js",

Updated the list of scripts.

Yes, if the noscripts version is the only version, the list would only need to exist in this file. However, if we want to have the ability to toggle the loading of Activity Stream in `about:config` (`browser.tabs.remote.separatePrivilegedContentProcess`), then we would need to keep the non-noscripts version as well. The noscripts version will only be used if the privileged content process is enabled.

We can create a new file to export the list of scripts and include it in the omnijar. That way the list can be shared by both `render-activity-stream-html.js` and `aboutNewTabService.js`. If we were to go with this method, I'd prefer doing this in a follow-up patch.

Comment 121

[Jay Lim [:imjching]]

Comment on attachment 8987202 [details]
Bug 1416066 - Set default ProcessType in ScriptPreloader to Uninitialized and update version of startup script cache file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252446/diff/4-5/

Comment 122

[Jay Lim [:imjching]]

Comment on attachment 8988733 [details]
Bug 1416066 - Update script_cache.py to support new version of startup script cache file.

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

Comment 123

[Jay Lim [:imjching]]

Comment on attachment 8987203 [details]
Bug 1416066 - Delay initialization of sProcessType for child processes in ScriptPreloader since remoteType in ContentChild is not ready yet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252448/diff/4-5/

Comment 124

[Jay Lim [:imjching]]

Comment on attachment 8987205 [details]
Bug 1416066 - Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252452/diff/5-6/

Comment 125

[Jay Lim [:imjching]]

Comment on attachment 8989779 [details]
Bug 1416066 - Enable caching for scripts with codebase URLs of about:home, about:newtab, and about:welcome.

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

Comment 126

[Jay Lim [:imjching]]

Comment on attachment 8987206 [details]
Bug 1416066 - Use XPConnect compilation scope for some non-cached local scripts with codebase principal when preload cache is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252454/diff/5-6/

Comment 127

[Jay Lim [:imjching]]

Comment on attachment 8987208 [details]
Bug 1416066 - Add preference observer for browser.tabs.remote.separatePrivilegedContentProcess in aboutNewTabService.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252458/diff/5-6/

Comment 128

[Jay Lim [:imjching]]

Comment on attachment 8987210 [details]
Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252462/diff/6-7/

Comment 129

[Jay Lim [:imjching]]

Comment on attachment 8989780 [details]
Bug 1416066 - Add a new flag to nsIAboutModule to load URIs in privileged content processes if feature is enabled.

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

Comment 130

[Jay Lim [:imjching]]

Comment on attachment 8987203 [details]
Bug 1416066 - Delay initialization of sProcessType for child processes in ScriptPreloader since remoteType in ContentChild is not ready yet.

https://reviewboard.mozilla.org/r/252448/#review262936

::: js/xpconnect/loader/ScriptPreloader.cpp:365
(Diff revision 5)
>          MOZ_ASSERT(XRE_IsParentProcess());
>  
>          mStartupFinished = true;
>      } else if (!strcmp(topic, CACHE_WRITE_TOPIC)) {
>          obs->RemoveObserver(this, CACHE_WRITE_TOPIC);
> +

Sorry, this was here because I wanted to make it look like the block before this.

Comment 131

[Kris Maglione [:kmag]]

Comment on attachment 8987205 [details]
Bug 1416066 - Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes.

https://reviewboard.mozilla.org/r/252452/#review262890

> Why? Can we assume that `topic` will never be an empty string? `mContentStartupFinishedTopic` will not be initialized in the parent process.
> 
> Would asserting that it is a content process be better here?

Yes, we can assume that `topic` will never be an empty string. Even if we ever dispatched observers with an empty topic (which we shouldn't), we never register to observe them.

Comment 132

[Jay Lim [:imjching]]

Comment on attachment 8987205 [details]
Bug 1416066 - Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252452/diff/6-7/

Comment 133

[Jay Lim [:imjching]]

Comment on attachment 8989779 [details]
Bug 1416066 - Enable caching for scripts with codebase URLs of about:home, about:newtab, and about:welcome.

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

Comment 134

[Jay Lim [:imjching]]

Comment on attachment 8987206 [details]
Bug 1416066 - Use XPConnect compilation scope for some non-cached local scripts with codebase principal when preload cache is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252454/diff/6-7/

Comment 135

[Jay Lim [:imjching]]

Comment on attachment 8987208 [details]
Bug 1416066 - Add preference observer for browser.tabs.remote.separatePrivilegedContentProcess in aboutNewTabService.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252458/diff/6-7/

Comment 136

[Jay Lim [:imjching]]

Comment on attachment 8987210 [details]
Bug 1416066 - Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252462/diff/7-8/

Comment 137

[Jay Lim [:imjching]]

Comment on attachment 8989780 [details]
Bug 1416066 - Add a new flag to nsIAboutModule to load URIs in privileged content processes if feature is enabled.

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

Comment 138

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1733b8ea728c
Set default ProcessType in ScriptPreloader to Uninitialized and update version of startup script cache file. r=kmag
https://hg.mozilla.org/integration/autoland/rev/e925ddaab1e1
Update script_cache.py to support new version of startup script cache file. r=kmag
https://hg.mozilla.org/integration/autoland/rev/240f5af2f631
Delay initialization of sProcessType for child processes in ScriptPreloader since remoteType in ContentChild is not ready yet. r=kmag
https://hg.mozilla.org/integration/autoland/rev/5a44678ce67f
Make ScriptPreloader wait for content-document-loaded to fire before writing cache for privileged processes. r=kmag,mconley
https://hg.mozilla.org/integration/autoland/rev/c34b5c75fe07
Enable caching for scripts with codebase URLs of about:home, about:newtab, and about:welcome. r=kmag
https://hg.mozilla.org/integration/autoland/rev/6115894b34a5
Use XPConnect compilation scope for some non-cached local scripts with codebase principal when preload cache is enabled. r=kmag
https://hg.mozilla.org/integration/autoland/rev/2a40499258f1
Add preference observer for browser.tabs.remote.separatePrivilegedContentProcess in aboutNewTabService.js. r=k88hudson
https://hg.mozilla.org/integration/autoland/rev/2cc1b7bcca99
Use mozJSSubScriptLoader to inject Activity Stream scripts when DOMContentLoaded is fired. r=k88hudson
https://hg.mozilla.org/integration/autoland/rev/3b132403a631
Add a new flag to nsIAboutModule to load URIs in privileged content processes if feature is enabled. r=mconley

Comment 139

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/1733b8ea728c
https://hg.mozilla.org/mozilla-central/rev/e925ddaab1e1
https://hg.mozilla.org/mozilla-central/rev/240f5af2f631
https://hg.mozilla.org/mozilla-central/rev/5a44678ce67f
https://hg.mozilla.org/mozilla-central/rev/c34b5c75fe07
https://hg.mozilla.org/mozilla-central/rev/6115894b34a5
https://hg.mozilla.org/mozilla-central/rev/2a40499258f1
https://hg.mozilla.org/mozilla-central/rev/2cc1b7bcca99
https://hg.mozilla.org/mozilla-central/rev/3b132403a631