1372664 - browser_messagemanager_loadprocessscript.js fails when Activity Stream is enabled

Status: RESOLVED FIXED

(Firefox Graveyard :: Activity Streams: General, defect)

Description

[Kate Hudson :k88hudson]

When running the test suite in preparation to pref Activity Stream on, the some tests fail in browser_messagemanager_loadprocessscript.js with messages liek this:

TEST-UNEXPECTED-FAIL | dom/base/test/browser_messagemanager_loadprocessscript.js | Should get back to 3 processes at this point. - Got 4, expected 3

Comment 1

[Kate Hudson :k88hudson]

Attachment: Bug 1372664 - Fix browser_messagemanager_loadprocessscript.js to pass when Activity Stream is enabled

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

Comment 2

[Kate Hudson :k88hudson]

Does this look like an appropriate fix to you? What are the 3 child processes in this test actually referring to? I'm still not exactly clear on how individual child processes get created etc. so any guidance here would be much appreciated :P

For context, we are preparing to pref on the activity-stream system add-on, which unlike the existing new tab page is unprivileged with URI_MUST_LOAD_IN_CHILD set to true.

Comment 3

[Ed Lee :Mardak]

We believe this number changes because of bug 1365643 but the pref is false by default.

Comment 4

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8877250 [details]
Bug 1372664 - Fix browser_messagemanager_loadprocessscript.js to pass when Activity Stream is enabled

https://reviewboard.mozilla.org/r/148600/#review153436

(In reply to Kate Hudson :k88hudson from comment #2)
> Does this look like an appropriate fix to you? What are the 3 child
> processes in this test actually referring to? I'm still not exactly clear on
> how individual child processes get created etc. so any guidance here would
> be much appreciated :P

Sorry, but I'm affraid not. The 3 processes here are the parent process and
2 content processes for the two opened tabs of the test at this point (one of
them might be a test window I cannot recall it right now sorry). The problem is that
the patch caused this side effect of having one more content process alive probably
always, which is causing a significant memory overhead and I don't understand why are
we OK with that.

My theory is that since the patch forces about:newTab to run in a content process the
preloaded browser [1] will come now with a side effect of always keeping alive an extra
content process in the background.

[1]: http://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/browser/base/content/tabbrowser.xml#2003

> For context, we are preparing to pref on the activity-stream system add-on,
> which unlike the existing new tab page is unprivileged with
> URI_MUST_LOAD_IN_CHILD set to true.

(In reply to Ed Lee :Mardak from comment #3)
> We believe this number changes because of bug 1365643 but the pref is false
> by default.

The test in the patch has a bug:
Services.prefs.setBoolPref("browser.newtabpage.activity-stream.enabled", true);

if you do this that means the pref will stay set for all the rest of the tests that follows this one.
You should use something like SpecialPowers.pushPrefEnv instead.

The bigger problem is that once this flag is turned on it will keep alive an extra content process
as I described above, which should be adressed first.

Comment 5

[Gabor Krizsanits (INACTIVE)]

I forgot to clear the needinfo flag.

Comment 6

[Kate Hudson :k88hudson]

Ok, that make sense. We do need to both preload the new tab (so that it appears quickly when new tabs are opened) as well have it running in a content process when we ship it, so perhaps could merge strategies to take advantage of content process preloading (i.e. run the preloaded new tab in the preloaded content process). Do you think that might be possible?

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Kate Hudson :k88hudson from comment #6)
> Ok, that make sense. We do need to both preload the new tab (so that it
> appears quickly when new tabs are opened) as well have it running in a
> content process when we ship it, so perhaps could merge strategies to take
> advantage of content process preloading (i.e. run the preloaded new tab in
> the preloaded content process). Do you think that might be possible?

Oh, sorry I have not seen your reply earlier, thanks for the needInfo flag it really helps :)

I would like to avoid having two content processes in the bg at the same time for caching puposes. The current content process that is held by the preallocated process manager can be used both in this case, and in case the user opens some link with right click in a new tab, or for new windows, etc.

We could let the preloaded browser to use the preallocated process maybe, it's gonna be tricky and a bit ugly. But could work, if we make sure that:
1. once the tab is no longer in the bg and its browser is inserted and rendering, we tell the preallocated process manager to forget that process and start up a new one
2. plus if something else starts using that process, and the ppm start up a new process, we can tell the preloaded browser to try and pick that one when it loads some new content.

I'm trying to think up something better. But first, could you provide me some context about why is it necessary to have the preloaded browser to run in a content process on the first place?

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

The reason we moved this to the content process in the first place was twofold:

* we've been told repeated that we should have as little code running on the main process as is reasonably possible, in order to free up the main thread for the most critical tasks

* this is implemented as a content page, so it seemed like the logical place to put it.

FWIW, the current implementation of the preloaded browser means that after the first about:newtab in a window opens, there should always be one newtab cached for the new about:newtab opening in that window.  

https://bugzilla.mozilla.org/show_bug.cgi?id=1077652 was where browser preloading was initially implemented.  Much of the implementation lives in tabbrowser.xml.

Does that help?

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Dan Mosedale (:dmose) from comment #8)
> The reason we moved this to the content process in the first place was
> twofold:
> 
> * we've been told repeated that we should have as little code running on the
> main process as is reasonably possible, in order to free up the main thread
> for the most critical tasks
> 
> * this is implemented as a content page, so it seemed like the logical place
> to put it.
> 
> FWIW, the current implementation of the preloaded browser means that after
> the first about:newtab in a window opens, there should always be one newtab
> cached for the new about:newtab opening in that window.  
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1077652 was where browser
> preloading was initially implemented.  Much of the implementation lives in
> tabbrowser.xml.
> 
> Does that help?

It does make sense to put about:newtab in a content process but if these are the only reasons to do so (so it's not a security driven decision) then we can:
- use an existing content process instead a new one to avoid keeping alive an extra process only for preloaded browser
- not block Activity Stream on figuring out a reasonable solution for this problem

Since front-end code is not really my expertise I would like to get some input from Mike. Mike, which version would make the most sense to you here (Comment 7, the alternatives in this comment, or something else)?

Also, I think we should ask a memory expert as well about the importance of this problem. Eric, the problem in a nutshell here is that for Activity Stream the current version will keep alive an extra content process for the preloaded browser. That means that in a single tab case, we will end up having one extra content process for the preallocated process and one other for the preloaded browser. Ofc with 3 opened tabs and above this would mean no memory regression, but I'm worried about the 1-2 tab cases since those are very common cases according to telemetry. I think this should be a blocker, but I might be too strict here, could you share your view on this?

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

So, to be clear, I completely agree that it can and should live in a normal content process.  When I added URI_MUST_LOAD_IN_CHILD for the page to AboutRedirector.cpp, my expectation was that that would cause it to live in a normal content process.  The fact that a separate content process was spun up for it was a surprise to me. 

My understanding of comment 9 is that we don't need to block preffing on Activity Stream in Nightly on getting the optimal solution to this bug, which is great, because we are hoping that we'll be able to pref it on Nightly this week.  That said, we will need to somehow tweak the test so that it doesn't turn the tree orange when we pref on.

Comment 11

[Gabor Krizsanits (INACTIVE)]

(In reply to Dan Mosedale (:dmose) from comment #10)
> My understanding of comment 9 is that we don't need to block preffing on
> Activity Stream in Nightly on getting the optimal solution to this bug,
> which is great, because we are hoping that we'll be able to pref it on
> Nightly this week.  That said, we will need to somehow tweak the test so
> that it doesn't turn the tree orange when we pref on.

For that I think the easiest solution is to pref it off for this specific test, and file a follow up bug to remove that line from the test later. Only problem with that plan is that turning the pref on will regress both AWSY and some Talos tests that measure memory footprint so I suggest you to consult with Eric about it first to avoid a back-out.

Comment 12

[Gabor Krizsanits (INACTIVE)]

Ofc without moving about:newTab to a content process, there won't be any regression, and this test will not need any fixes either if that's an acceptable solution for now.

Comment 13

[Kate Hudson :k88hudson]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #12)
> Ofc without moving about:newTab to a content process, there won't be any
> regression, and this test will not need any fixes either if that's an
> acceptable solution for now.

Like Dan said, I think it would be totally fine to be in a normal content process, the extra content process was somewhat unexpected. Personally I'd prefer to pref off activity-stream just for this test instead of moving about:newtab back to the main process, since we've already landed some fixes for tests specifically around about:newtab being in a content process and we'd have to back those out.

Thanks!

Comment 14

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

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> 
> Since front-end code is not really my expertise I would like to get some
> input from Mike. Mike, which version would make the most sense to you here
> (Comment 7, the alternatives in this comment, or something else)?
> 

I think we should either:

a) Load the preloaded about:newtab in the preallocated content process (and keep that preallocated content process in the preallocated state), OR
b) Find an existing content process, preferably one that's not being used by the foreground tab (to avoid jank in it while loading Activity Stream). This one has some edge cases, since there might not _be_ a content process except for the preallocated one (for example, a single tab might be pointed at about:addons or something).

The danger with (b) is that we end up getting into the situation where the preallocated process is rarely used, since every preloaded newtab will use an existing (non-preallocated process), most "new tab and browse" actions by the user will end up using a pre-existing content process rather than the preallocated one.

So I think I prefer (a), simply because we know that this process will not be doing much anyways, and loading Activity Stream in it will not cause jank in pre-existing tabs. I do realize, however, that this will require some plumbing to accomplish.

Comment 15

[Gabor Krizsanits (INACTIVE)]

(In reply to Mike Conley (:mconley) from comment #14)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> > 
> > Since front-end code is not really my expertise I would like to get some
> > input from Mike. Mike, which version would make the most sense to you here
> > (Comment 7, the alternatives in this comment, or something else)?
> > 
> 
> I think we should either:
> 
> a) Load the preloaded about:newtab in the preallocated content process (and
> keep that preallocated content process in the preallocated state), OR
> b) Find an existing content process, preferably one that's not being used by
> the foreground tab (to avoid jank in it while loading Activity Stream). This
> one has some edge cases, since there might not _be_ a content process except
> for the preallocated one (for example, a single tab might be pointed at
> about:addons or something).
> 
> The danger with (b) is that we end up getting into the situation where the
> preallocated process is rarely used, since every preloaded newtab will use
> an existing (non-preallocated process), most "new tab and browse" actions by
> the user will end up using a pre-existing content process rather than the
> preallocated one.
> 

In this version I think what we can do is that the first time the user navigates away from about:newtab we open that page in another content process (which will either be the preallocated cp or an already existing one, but yeah it's a bit tricky).

> So I think I prefer (a), simply because we know that this process will not
> be doing much anyways, and loading Activity Stream in it will not cause jank
> in pre-existing tabs. I do realize, however, that this will require some
> plumbing to accomplish.

It has it quirks as well. We need to deal with the case when something else start using the preallocated process... I think even in this version I would prefer to select a new content process after once the user is navigating away from about:newtab.

Anyway, it feels like in both case the right thing to do is making about:newtab a special case only, and let the first navigation from there use the default process selection algorithm (which could potentially pick or re-pick the preallocated content process). Once we have that it really does not matter where we put about:newtab.

Comment 16

[Ed Lee :Mardak]

I tried setting +pref("dom.ipc.processPrelaunch.enabled", false); and try still fails:

https://hg.mozilla.org/try/rev/7b4b1892262676ebd9e18b40ae4a8026389aafba
https://treeherder.mozilla.org/logviewer.html#?job_id=110733059&repo=try&lineNumber=4260

The test is already forcing the pref to false:
https://dxr.mozilla.org/mozilla-central/source/dom/base/test/browser_messagemanager_loadprocessscript.js#53-55

Setting the default pref via app/profile/firefox.js for browser.newtab.preload to false and running the one test has it passing with the tested 3 processes.

gabor, should k88hudson go with the original patch or try turning off preload and unload the tab + process?

Comment 17

[Kate Hudson :k88hudson]

I'm finding that the extra content process is sticking around even with pref being turned off and the preloaded tab released – maybe a race condition? If we decide to go that route, perhaps we could open a new window to run the tests in to get around this problem?

Comment 18

[Gabor Krizsanits (INACTIVE)]

(In reply to Ed Lee :Mardak from comment #16)
> The test is already forcing the pref to false:

Yeah, we just need to fix the test. Let me take a second look at the patch.

Also, I'm clearing needinfo from :erahm too since we have a plan here.

Comment 19

[Gabor Krizsanits (INACTIVE)]

(In reply to Ed Lee :Mardak from comment #16)
> gabor, should k88hudson go with the original patch or try turning off
> preload and unload the tab + process?

Since the process count will be depending on the preloaded browser(s) which is a bit undeterministic. Can we just turn off activity stream for this test here: http://searchfox.org/mozilla-central/source/dom/base/test/browser_messagemanager_loadprocessscript.js#55?

Comment 20

[Ed Lee :Mardak]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19)
> Since the process count will be depending on the preloaded browser(s) which
> is a bit undeterministic. Can we just turn off activity stream for this test
It looks like the preloaded activity-stream will have already caused another content process to load, so disabling won't cause the process count to drop back down.

Should the test then just check for activity-stream being enabled and allowing for either 3 or 4 ppmm.childCount instead of strictly 3?

Comment 21

[Ed Lee :Mardak]

Attachment: Bug 1372664 - browser_messagemanager_loadprocessscript.js fails when Activity Stream is enabled.

Allow for either the usual 3 base process count or with activity-stream both 3 or 4 counts.

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

Comment 22

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8883791 [details]
Bug 1372664 - browser_messagemanager_loadprocessscript.js fails when Activity Stream is enabled.

https://reviewboard.mozilla.org/r/154748/#review160616

Sorry for the lag, I was on PTO... This looks fine until we find a real solution for the problem.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c54e7d29632
browser_messagemanager_loadprocessscript.js fails when Activity Stream is enabled. r=krizsa

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c54e7d29632