807438 - [e.me][everything.me] elle view full site can cause the phone to reboot.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Everything.me, defect, P2)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

## Environment :
Unagi phone, build 20121031073004
2012-10-31 10:23:10
  
## Repro :
1. swipe to e.me
2. select fashion
3. select elle fashion magazine
4. pan down and select "view full site"
5. hit home button
6. select elle fashion magazine again

## Expected :
1. elle full site

## Actual :
1. phone reboots

## Note :
1. trying to repro bug 798700

Comment 1

[Dylan Oliver [:doliver]]

blocking-

We've seen several bugs raised against Elle -- not clear if that app in particular is problematic and should be removed, or if there's some larger issue to be investigated.

Comment 2

[Ran Ben Aharon [:ranbena] EverythingMe]

Guys, this is an issue related to the ui-wrapper browser for it has to do with loading and running the Elle site.
Please re-assign this issue. I don't know to whom - perhaps have [e-mail]?

Comment 3

[Justin Lebar (not reading bugmail)]

> Guys, this is an issue related to the ui-wrapper browser for it has to do with loading 
> and running the Elle site.

Yeah; we shouldn't be able to crash the main process.

Is the e.me browser running in the main process (i.e., is it non-remote)?  This bug seems like clear evidence we should not be doing that.

Comment 4

[Ben Francis [:benfrancis]]

e.me content runs in the homescreen process, not the system process, so I'm surprised it can reboot the phone.

Comment 5

[Ben Francis [:benfrancis]]

Naoki says this can be reliably reproduced and it crashes the phone so it's a blocker.

A content crash should crash the homescreen process, not reboot the whole device so it seems there's something deeper going on here. Needs further investigation.

Comment 6

[Justin Lebar (not reading bugmail)]

Andrea, here's the mysterious full-phone crash I mentioned a few days ago.  Like Ben says, we should not be crashing the main process here; that's quite bad.

Comment 7

[Alex Keybl [:akeybl]]

Milestoning for C2 (deadline of 12/10), as this meets the criteria of "known P2 bugs found before or during C1".

Comment 8

[Alexandre Poirot [:ochameau]]

Is anyone of you still be able to reproduce this crash?
Because I'm not. The homescreen is killed and restarted because of memory pressure, but other than that, it seems to work fine.

Comment 9

[JP Rosevear [:jpr]]

Naoki, can you still reproduced?

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I can't reproduce, but the STR in comment 0 bring the homescreen to its knees, which is bug 811315 and friends.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 6764 - e10ify wrapper

Here is the gaia patch to use system app content processes for e.me and homescreen bookmark apps.
I ended up highlighting this potential, non-ideal, but still better than current behavior, in bug 811315 comment 50.
It will avoid crashing the homescreen when such app crash, and avoid some more or less heavy lags on homescreen when opening these same apps.

Note that I'm still interested in steps to reproduce crashes in order to see how this patch improves on that scenario.

Chris, Can you take a look at this patch and see if mozbrowserdialog implementation is correct regarding platform behavior?
There is mainly system app machinery in this patch in order to keep opening e.me apps in a single frame and bookmark apps in multiple ones.
But I think you will be most likely interested in this code:
  https://github.com/mozilla-b2g/gaia/pull/6764/files#L0R1447

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 687302 [details]
Pull request 6764 - e10ify wrapper

The approach seems ok, although I'm sad we have to hack window.open for this.  It's really expensive.

How is "only the homescreen is opening these windows" enforced?

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #12)
> Comment on attachment 687302 [details]
> Pull request 6764 - e10ify wrapper
> 
> The approach seems ok, although I'm sad we have to hack window.open for
> this.  It's really expensive.

The main issue is that using Activities will let web site popups remote window :/
> 
> How is "only the homescreen is opening these windows" enforced?

There is a global mozbrowserwindowopen listener that returns for non-homescreen window and do something special when the request come from a homescreen window. See https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/window_manager.js#L1387

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 687302 [details]
Pull request 6764 - e10ify wrapper

r+. Do we need a platform patch in order to land that? (for the permission?).

If yes I would say let's add a workaround for now in the code and open a followup so we can land it.

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Bug 807438: Add new `open-remote-window` app permission.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 689182 [details] [diff] [review]
Bug 807438: Add new `open-remote-window` app permission.

Jonas, here is a gecko patch to add a new `open-remote-window` permission in order to allow the homescreen to open a URL in a remote process.

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 689182 [details] [diff] [review]
Bug 807438: Add new `open-remote-window` app permission.

Review of attachment 689182 [details] [diff] [review]:
-----------------------------------------------------------------

Jonas, this is the followup of the discussion we got in SF on friday evening.

Comment 18

[Alexandre Poirot [:ochameau]]

Checkin needed for attachment 689182 [details] [diff] [review].
Then, we will be able to land the pull request.

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I'm changing the component to reflect the fact that the patch touch PermissionInstaller.jsm and that the changes needs to land on Aurora/Beta.

https://hg.mozilla.org/integration/mozilla-inbound/rev/64dabdd4df4a

Comment 20

[Alexandre Poirot [:ochameau]]

I originaly used this bug because bug 811315 changed from Gaia to General.
We can't keep up changing bug component like this.
I opened a platform bug for attachment 689182 [details] [diff] [review], it will uplift branches there.

Comment 21

[Alexandre Poirot [:ochameau]]

The main patch of this bug, the gaia pull request 6764 landed:
https://github.com/mozilla-b2g/gaia/commit/e03724a69f7989e6a16016daa0df96e3057f2ccf

With the homescreen permission being hardcoded until proper 'open-remote-window' permission.

Comment 22

[Gregor Wagner [:gwagner]]

Why do we merge the gaia changes before the gecko change hits mc? This results in a bunch of permission install errors on mc.

Comment 23

[Cristian Rodriguez (:crdlc)]

there is a small regression here, please read https://github.com/mozilla-b2g/gaia/pull/6764#discussion-diff-2367661

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64dabdd4df4a

Comment 25

[Cristian Rodriguez (:crdlc)]

other regression: the history is shared between different apps launched from ev.me search

1) go to Facebook
2) close facebook
3) go to twetter
4) open footer and click back -> go to Facebook (but the back should be disabled)

the problem is the same than before, reusing the same browsing context

Comment 26

[Alexandre Poirot [:ochameau]]

(In reply to Gregor Wagner [:gwagner] from comment #22)
> Why do we merge the gaia changes before the gecko change hits mc? This
> results in a bunch of permission install errors on mc.

I don't think it was due to this patch but more because of bug 812198.
But at the end you are right, it failed for the exact same reason :/

Comment 27

[Cristian Rodriguez (:crdlc)]

yes, I agree it fails for the same reason but IMHO the regression has been introduced here, because before, we closed the previous window and opened other one [1]

[1] https://github.com/mozilla-b2g/gaia/pull/6764/files#L0L16

thanks

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Ryan VanderMeulen from comment #24)
> https://hg.mozilla.org/mozilla-central/rev/64dabdd4df4a

Hye Ryan can you push the platform patch to aurora/beta as well? Sorry we let it in the Gaia component so I guess it has been lost.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Absolutely. I'll get it with my next round of uplifts.

Comment 30

[Daniel Coloma:dcoloma]

Should this bug still be in OPEN status? In general, should we only mark a b2g bug as fixed when the related gecko parts land in beta?

Comment 31

[Alexandre Poirot [:ochameau]]

As I said in bug 820305 comment 13, and as comment 21 of this bug suggest, 
the gaia patch **DOES NOT DEPEND ON GECKO PATCH**.
As I've done what Vivien suggested in comment 14 during the review.

So let me explain you what we have here as it seems to be unclear.
The gaia patch only allows homescreen app to open OOP windows, it doesn't try to search for any permission. So that the gaia patch doesn't depend on gecko patch anymore.
The gecko patch from bug 819884 will just allow other apps to do so and will need another gaia modification via bug 819882.

And note that the patch already reached beta, in bug 819884.
So Ryan, you can ignore comment 29.

Comment 32

[Justin Lebar (not reading bugmail)]

(In reply to Daniel Coloma:dcoloma from comment #30)
> Should this bug still be in OPEN status? In general, should we only mark a
> b2g bug as fixed when the related gecko parts land in beta?

We mark Gecko bugs as fixed when they reach mozilla-central.  We use the status-firefoxN flags to indicate when a bug has been fixed on branches.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

(In reply to Alexandre Poirot (:ochameau) from comment #31)
> And note that the patch already reached beta, in bug 819884.
> So Ryan, you can ignore comment 29.

In that case, setting the flags to fixed. Thanks.

Comment 34

[Rebecca Billings [:rbillings]]

Unable to repro on the 12/28 unagi build.