Closed Bug 807438 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

VERIFIED FIXED
B2G C2 (20nov-10dec)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: nhirata, Assigned: ochameau)

References

Details

(Keywords: unagi, Whiteboard: [b2g-crash])

Attachments

(2 files)

## 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
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.
Assignee: nobody → ran
blocking-basecamp: ? → -
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 crdlr@tid.es?
> 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.
blocking-basecamp: - → ?
e.me content runs in the homescreen process, not the system process, so I'm surprised it can reboot the phone.
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.
blocking-basecamp: ? → +
Priority: -- → P2
Component: Gaia → Gaia::Homescreen
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.
Milestoning for C2 (deadline of 12/10), as this meets the criteria of "known P2 bugs found before or during C1".
Target Milestone: --- → B2G C2 (20nov-10dec)
Component: Gaia::Homescreen → Gaia::Everything.me
Component: Gaia::Everything.me → Gaia::Homescreen
Depends on: 811315
Component: Gaia::Homescreen → Gaia::Everything.me
Assignee: ran → poirot.alex
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.
Assignee: poirot.alex → ran
Naoki, can you still reproduced?
Flags: needinfo?(nhirata.bugzilla)
Keywords: qawanted
I can't reproduce, but the STR in comment 0 bring the homescreen to its knees, which is bug 811315 and friends.
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
Assignee: ran → poirot.alex
Attachment #687302 - Flags: feedback?(jones.chris.g)
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?
Attachment #687302 - Flags: feedback?(jones.chris.g) → feedback+
(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
Attachment #687302 - Flags: review?(21)
No longer blocks: 812972
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.
Attachment #687302 - Flags: review?(21) → review+
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.
Attachment #689182 - Flags: review?(jonas)
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.
Checkin needed for attachment 689182 [details] [diff] [review].
Then, we will be able to land the pull request.
Keywords: checkin-needed
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
Status: NEW → ASSIGNED
Component: Gaia::Everything.me → General
Keywords: checkin-needed
Depends on: 819882
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.
Component: General → Gaia::Everything.me
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.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Why do we merge the gaia changes before the gecko change hits mc? This results in a bunch of permission install errors on mc.
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
(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 :/
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
Depends on: 820100
(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.
Flags: needinfo?(ryanvm)
Absolutely. I'll get it with my next round of uplifts.
Flags: needinfo?(ryanvm)
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?
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.
(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.
(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.
Unable to repro on the 12/28 unagi build.
Flags: needinfo?(nhirata.bugzilla)
Status: RESOLVED → VERIFIED
Keywords: qawanted
Duplicate of this bug: 824034
You need to log in before you can comment on or make changes to this bug.