836647 - The B2G browser app should have at most one (or, if you must, two) foreground processes

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect)

Description

[Justin Lebar (not reading bugmail)]

Right now the browser app holds each new tab alive until it takes a screenshot of it.

This is one of the things I think we need to fix for bug 836199 and bug 836638.  The fact that we have many fg tabs in the browser means that it's hard for the communications app to get the CPU cycles it needs to start up.  And many fg tabs also make it difficult to keep the communications app from being killed on low memory.

I have a patch for this, but it doesn't work.  I'll describe what's going on in my next comment.

Comment 1

[Justin Lebar (not reading bugmail)]

I'm running into a lot of trouble trying to fix this bug.

My simple patch [1] sends tabs to the background when they're created by the
'View' activity.  It also doesn't hold tabs in the foreground waiting for a
screenshot.

The latter part seems to work.  (It messes up screenshots, but that's
expected.)  The former change, however, causes subtle breakage that I haven't
been able to figure out.

When I run membuster's "bust tabs" with the hideCurrentTab() call from this
patch in place, some tabs that the test opens don't load when I focus them.
They just stay as a white page.

There are a variety of failure modes here.  Sometimes the page appears
immediately if I go back to the tab strip and re-select the active tab.  Other
times, the APZC seems to get confused, and I see TabChild::RecvUpdateFrame send
Viewport:Change with a JSON argument like

> { "x" : 0, "y" : 0,
>  "viewport" : { "width" : 980.000000, "height" : 1117.812500 },
>  "displayPort" : { "x" : -0.000005, "y" : 0.000000, 
>                    "width" : 980.000000, "height" : 1117.000000 },
>  "compositionBounds" : { "x" : 0, "y" : 0, "width" : 320, "height" : 365 },
>  "cssPageRect" : { "x" : 0.000000, "y" : 0.000000, "width" : 980.000000, "height" : 1117.000000 },
>  "cssCompositedRect" : { "width" : 980.000000, "height" : 1117.000000 }
> }

(TabChild::RecvUpdateFrame is in this case being triggered by
TabParent::UpdateFrame, which is ultimately triggered from APZC
RequestContentPaint, I think.)

I think what's messed up about this is that the displayPort width and height
should be (320, 365).

When this happens, no amount of re-selecting the tab fixes the problem.  If I
try to pan around, I can only move the x and y values to -1, 0, and 1.

Other times cssCompositedRect's (width, height) values are (0,0) or (NaN, NaN).
The latter isn't valid JSON.  :)

If I turn off APZ, I still get the white pages.

The frame in question is almost definitely being sent to the foreground when I
activate its tab, so what seems to be happening is that loading it in the
background is causing problems.  Or maybe we try to draw it before the fg
change takes effect.  At least, that's my best guess.

This isn't a problem that has to do with crashed frames.  Or at least, the
frame in question is not crashed when it's showing the white screen.

My hope is that I've messed something up here and that the patch I want for the
browser app is actually simple.  Ben or Dale, I would really appreciate if you
could take a close look at this patch and let me know if it looks wrong to you.
Perhaps it wasn't a simple mistake that we didn't have a hideCurrentTab() call
in the 'View' handler.

I'd appreciate even more if you could hack on this patch and try to make it
work for you.  I'm doubly out of my depth here debugging Gaia + Layers.

[1] https://github.com/jlebar/gaia/commit/c5a9b227f17b287ecdd7b9522404fe8389f37ee8

Comment 2

[Andrew Overholt [:overholt]]

tef? since jlebar thinks it blocks bug 836199 which is tef+.

Comment 3

[Andrew Overholt [:overholt]]

Blocker because it blocks bug 836199.

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

Taken

Comment 5

[Steve Chung [:steveck]]

Hi Justin, I can take a look first. Thanks for your elaboration.

Comment 6

[Steve Chung [:steveck]]

I've tried out the patch but I didn't see any significant failure except for the screen shoot issue. I'm curious about your membuster test for tab. Do you write a simple script like the membuster in test app, or any existing profiling api/helper function for memory?

Comment 7

[Justin Lebar (not reading bugmail)]

The membuster page is http://people.mozilla.com/~cjones/membuster.html.  We're using the "bust processes" button.

Comment 8

[Steve Chung [:steveck]]

I still could not reproduce the issue. Sometimes the tab shows white page at first, but it will start to load the content about a second later. My gecko build is b2g18 with commit e2abfafb7d88df1ed6bf8bcfedb4b0a5dafb7313 and gaia: 5b9a3d901deba79584f0a03b732f2b360686b652. Sorry that I couldn't provide any useful information for your problem.

Comment 9

[Justin Lebar (not reading bugmail)]

The white-page issue arises only with my Gaia patch, which I provided in comment 1.  Did you have that patch applied?

Comment 10

[Steve Chung [:steveck]]

Yes, I've applied the commit based on 5b9a3d901deba79584f0a03b732f2b360686b652, and this issue in not reproducible in my environment.

Comment 11

[David Scravaglieri [:scravag]]

Ben what is the status there ?

Comment 12

[Ben Francis [:benfrancis]]

I'm working on this, just trying to stop it breaking tab thumbnails and should have a new patch soon.

Comment 13

[Ben Francis [:benfrancis]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/8082

I've rolled an even simpler patch than Justin's which actually doesn't put tab processes in the foreground at all in order to screenshot them.

It appears that an iframe can still have a screenshot taken of it even if it has both document.mozHidden set to true and display set to none. This surprises me, so I'd like to get feedback from Justin on the sanity of this.

The patch also includes Justin's fix for Web Activities.

Comment 14

[Alex Keybl [:akeybl]]

We're continuing to block on this, as it blocks bug 836199. If at any point it's not clear that this continues to block bug 836199, or only has minimal gains, please let us know.

Comment 15

[Justin Lebar (not reading bugmail)]

Ben, lest you think I'm ignoring you: I'm currently testing this patch and have already filed two follow-ups to bug 836654 based on these tests.

Things still aren't working quite right, but I'm not yet sure if it's the fault of this patch.

Comment 16

[Justin Lebar (not reading bugmail)]

Comment on attachment 713444 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/8082

I'm happy with this, based on my testing.

There's still a fairly long period of time when browser processes are in the fg, during the process-buster testcase.  I haven't had a chance to investigate that.

It would be great if we could merge this soon.  Dale, have you had a chance to look at it?

Comment 17

[Dale Harvey (:daleharvey)]

Comment on attachment 713444 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/8082

Merged in: https://github.com/mozilla-b2g/gaia/commit/bae26c4a96b371b794b4433784410c1ec69c5093

Do tabs need to be explicitly told to be in the foreground or background? if they default to the foreground then we need to remember to setVisible(false) on them

Comment 18

[Justin Lebar (not reading bugmail)]

They default to the fg.

Comment 19

[Dale Harvey (:daleharvey)]

ok I will send a follow up

Comment 20

[Justin Lebar (not reading bugmail)]

Can you let me know when the follow-up lands (or cc me or whatever) so I can test whether that fixes some of the issues from comment 16?

Comment 21

[Dale Harvey (:daleharvey)]

I will do

One thing is that if the default to fg then there will be no way for us to limit the fg processes to 1, since if you open a tab from the context menu on the current page, the current page will be fg as well as the opened tab until it gets setVisibled

Comment 22

[Justin Lebar (not reading bugmail)]

My thought is that when we move mozbrowser into its own HTML tag, we'll be able to setVisible() before it loads, which will solve this problem.  But for now, I think we're basically stuck with this, like you say.

Anyway I don't think two fg processes will be a big deal.

Comment 23

[Dale Harvey (:daleharvey)]

Sorry this wasnt resolved, newly created tabs default to the foreground so we actually held open a 'lot' of foreground tabs, especially if you use the context menu + open in new tab

I have a follow up

Comment 24

[Dale Harvey (:daleharvey)]

Attachment: Follow up, default tabs to background

Comment 25

[(inactive after 6/18) Alive Kuo [:alive]]

I see 
E/GeckoConsole(  109): [JavaScript Error: "TypeError: tab is null" {file: "app://browser.gaiamobile.org/js/browser.js" line: 1122}]
E/GeckoConsole(  109): [JavaScript Error: "TypeError: this.currentTab is null" {file: "app://browser.gaiamobile.org/js/browser.js" line: 503}]

When I call `view` MozActivity to make browser to show a web page. Is this triggered by the first patch?

Comment 26

[Dale Harvey (:daleharvey)]

Alive, yeh you are right, sorry for missing that in review, have ammended the commit to address that

Comment 27

[Dale Harvey (:daleharvey)]

And for some reason github seems to be losing comments, so I also took your advice justin and did the sync setvisible if it exists.

The failing test is unrelated to this code

Comment 28

[Ben Francis [:benfrancis]]

Comment on attachment 715811 [details]
Follow up, default tabs to background

After speaking to Dale in IRC, I really don't like the wrapper function with setTimeout in it, but until we have an event from the browser API to know when the setVisible method is available to call it seems we have no option.

r+me

Comment 29

[Justin Lebar (not reading bugmail)]

> but until we have an event from the browser API to know when the setVisible method is available to 
> call it seems we have no option.

My thought is that I'm going to make the browser API available immediately on frames.  In order to do that, I'm going to have to introduce a new tag.

This will fix the setVisible issue, and it will also (hopefully) fix the issue where we sometimes miss a mozbrowserclose event.

I'm sorry this is so broken.

Comment 30

[Dale Harvey (:daleharvey)]

Merged in: https://github.com/mozilla-b2g/gaia/commit/b4beb05b81ad2fd3d6851a0e87757e7d18e7e72c

Comment 31

[John Ford [:jhford] CET/CEST Berlin Time]

Commit b4beb05b81ad2fd3d6851a0e87757e7d18e7e72c does not apply to v1-train or v1.0.1.  This means that there are merge conflicts which need to be resolved.  If there are dependencies that are not approved for branch landing, or have yet to land on master, please let me know

If a manual merge is required, a good place to start might be:
  cd gaia
  git checkout v1-train
  git cherry-pick -x -m1 b4beb05b81ad2fd3d6851a0e87757e7d18e7e72c
  <RESOLVE MERGE CONFLICTS>
  git checkout v1.0.1
  git cherry-pick -x $(git log -n1 v1-train)

Comment 32

[Dale Harvey (:daleharvey)]

https://github.com/mozilla-b2g/gaia/commit/bae26c4a96b371b794b4433784410c1ec69c5093

will also need uplifted

Comment 33

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Dale Harvey (:daleharvey) from comment #32)
> https://github.com/mozilla-b2g/gaia/commit/
> bae26c4a96b371b794b4433784410c1ec69c5093
> 
> will also need uplifted

Dale, the first patch hasn't been uplifted yet because of the merge conflicts mentioned in comment 31.

Comment 34

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train and v1.0.1.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train and v1.0.1, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 bae26c4a96b371b794b4433784410c1ec69c5093
  <RESOLVE MERGE CONFLICTS>
  git commit
  git checkout v1.0.1
  git cherry-pick -x $(git log -n1 v1-train)

Comment 35

[Justin Lebar (not reading bugmail)]

Ben, can you try uplifting this?

Comment 36

[Ben Francis [:benfrancis]]

I've managed to resolve the merge conflicts and have commits ready to uplift, but there's a problem.

It turns out that one of the commits in this bug caused a serious regression in the browser, which was later accidentally fixed in bug 849280. Bug 849280 was a fairly major refactor to add lazy-loading for performance improvements which was something QC said they could live without for 1.0.1. So if we do this uplift, we will also need to uplift bug 849280 which has some risk attached to it because of the size of the change.

This regression only seems to happen on the v1.0.1 branch, not v1-train although I'm not sure why because as far as I know bug 849280 hasn't been uplifted to v1-train yet.

Comment 37

[Ben Francis [:benfrancis]]

Uplifted to v1-train
https://github.com/mozilla-b2g/gaia/commit/3867f1689d0d17291098d1f54695feba2bd3266d

It seems this has in fact caused the regression with web activities in the browser after all. Luckily bug 849280 has now been approved for uplift to 1.1

Comment 38

[Ben Francis [:benfrancis]]

Apparently b2g18-status:fixed means "This has landed on Gaia v1-train"...

Comment 39

[John Ford [:jhford] CET/CEST Berlin Time]

Ben, to be clear, is bug 849280 a dependency for this bug landing on a given branch?

Comment 40

[Ben Francis [:benfrancis]]

It wouldn't be entirely accurate to call it a dependency. This patch introduced a regression which is fixed by bug 849280

Comment 41

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Ben Francis [:benfrancis] from comment #40)
> It wouldn't be entirely accurate to call it a dependency. This patch
> introduced a regression which is fixed by bug 849280

ok!  I'm following now

Comment 42

[Ben Francis [:benfrancis]]

bae26c4a96b371b794b4433784410c1ec69c5093 uplifted to v1.0.1 as ae63dfb3c501585b7b3ba93888311bd8e2d49448
b4beb05b81ad2fd3d6851a0e87757e7d18e7e72c also uplifted to v1.0.1 as ec12e3124af3d09aa512108ea71b70ca8f0c60b8 (still needs uplift to v1-train)

Comment 43

[Ben Francis [:benfrancis]]

b4beb05b81ad2fd3d6851a0e87757e7d18e7e72c uplifted to v1-train as 17c7750718734da8b5e5ba62f4d9d87a551610a0

Now properly status-b2g18:fixed !

Comment 45

[Cody Roesch [:croesch]]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 46

[amiller]

issue appears fixed in V1.0.1 and V1.1.0
- Only 2 tabs created display at one time. This is different from the previous behavior of multiple tabs displaying at one time. 

tested on Unagi running the following builds:
V1.0.1
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/ccec751a468e
Gaia   ee0bef61c0a25c806dd1eec5a4e047bc418a5f73
Build  2013-04-02-070202

V1.1.0
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/68c8a883cfc0
Gaia   1c38c91bb16f2bf0d5066c4787d2249463f61bb3
Build  2013-04-02-070204