920851 - Listen to mozbrowser events on iframes embedded within the trusted UI instead of on the trusted UI iframe itself

Status: RESOLVED FIXED

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

Description

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

If the Trusted UI is opened, and the OTA banner appears while the content inside the Trusted UI is loading, the Trusted UI will receive a mozbrowsererror event.

STR:

- Apply the patch in bug 903134 for trusted_ui to listen for mozbrowsererror
- reset gaia
- go through FTU and configure wifi
- go to UI Tests app and choose navigator.mozId
- click 'request'

Expected: The Persona dialog opens inside the Trusted UI (this occurs in a separate Browser process, if you are watching b2g-ps); the OTA banner appears.  Peace and harmony prevail.

Actual: The Persona dialog begins opening inside the Trusted UI (it loads content from persona.org into an iframe); as the OTA banner appears, the Trusted UI receives a mozbrowsererror event.

This is 100% repeatable for me.

If, however, you first go to a regular website (like google.com in the Browser app), the OTA will appear with evidently no adverse consequences.  Once the OTA banner has appeared, you can go to the navigator.mozId tests and open the Persona dialog.  The Trusted UI does not receive mozbrowsererror and everything works properly.

Also, if you do not apply the patch, so you are not listening for mozbrowsererror, and you go through the STR above, the Persona tests will work fine in the Trusted UI even though the OTA banner appears.  They do not crash or hang.

Build:
- b2g unagi eng
- gecko: mozilla-inbound @e5b5774cc1d1
- gaia: master @b3d003e1 (with patch from bug 903134 applied)

Comment 1

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

At the time when the event arrives, the adb logcat looks like this (with trusted_ui logging the type of each event it receives):

I/Gecko   (16502): ###!!! [Parent][AsyncChannel] Error: Channel error: cannot send/recv
I/GeckoDump(16502): trusted_ui: handleEvent: mozbrowsererror
I/GeckoDump(16502): trusted_ui: handleEvent: appterminated

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

What's the OTA banner? You mean the system update notification in the status bar?

Comment 3

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo, please) from comment #2)
> What's the OTA banner? You mean the system update notification in the status
> bar?

Yes, that's what I meant.  Have I got the right name for it?  Please change the title of this bug if I got it wrong.

Comment 4

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Hi, Etienne, I wonder if you have any ideas about this, or can point me to someone who can look at it with me?

Comment 5

[Etienne Segonzac (:etienne)]

Wow. Looks like a tricky one.
Note that the "ota banner" is displayed after a slight timeout so it might give you a false sense of correlation.

Otherwise the only person I can think off is the b2g-wildcard, Fabrice :)

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

I'll take a look on Monday

Comment 7

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Thanks Fernando, thanks Etienne.

Etienne, the reason I think there's a correlation is that if I take sign-in out of the FTU the banner does not appear; if I put it back in, it does appear.  I thought the timing of events could be tricking me, so I tried taking sign-in out of the FTU and then sitting and waiting for it to appear, but it doesn't.  I presume when the Persona content is loaded into the Trusted UI, the on-line event is triggered, the update manager hears it, and decides to request an update.

I wonder if this would happen with other hosted content in the FTU?  Contacts from Gmail, Facebook, etc.?

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

I couldn't reproduce this exact issue so far. What I've managed to reproduce is the trusted UI "crash" caused by an OOM crash of other process.

Specifically, what I've seen so far is that when the trusted UI loads the Persona flow, the Usage app is killed:

$ adb shell dmesg | grep kill
<4>[10-01 10:33:54.524] [28: kswapd0]select 3065 (Usage), adj 10, size 7189, to kill
<4>[10-01 10:33:54.524] [28: kswapd0]send sigkill to 3065 (Usage), adj 10, size 7189

which causes a 'mozbrowsererror' event that it is received by the system app:

I/Gecko   ( 2849): 
I/Gecko   ( 2849): ###!!! [Parent][AsyncChannel] Error: Channel error: cannot send/recv
I/Gecko   ( 2849): 
I/Gecko   ( 2849): BrowserElementParent.jsm - ******** FIRING FATAL ERROR [xpconnect wrapped nsIFrameLoader] null
I/Gecko   ( 2849): 
I/GeckoDump( 2849): trusted_ui: handleEvent: mozbrowsererror

which means that if a process is killed while loading the Persona flow (which seems quite possible), the trusted UI will show the crash message, even if the embedded content is ok.

It seems that we might need a different approach for bug 903134 or maybe some tweaks to the mozbrowser API, like adding extra information to the mozbrowsererror events (not sure how possible that is though).

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jed Parsons [:jedp, :jparsons] from comment #7)
> I wonder if this would happen with other hosted content in the FTU? 
> Contacts from Gmail, Facebook, etc.?

All the hosted content that is loaded from the FTU runs in the same process as the FTU, so I don't think this would ever happen.

The Persona flow runs in a different browser process, as every content loaded within the trusted UI (which is part of the main process).

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

It would be great if we could add to the "oop-frameloader-crashed" notification [1] the URL of the content loaded in the crashed frameloader.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#304

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Perhaps I don't understand, the mozbrowsererror is fired at the iframe of the process that crashed, right?  Shouldn't the system know what was in that iframe?

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

And of course you are right :). The problem is actually with how the trusted UI listens to mozbrowser* events. We should be listening to the mozbrowser events fired at the iframe embedded within the trusted UI, not at the trusted UI iframe itself.

Sorry for not seeing this before. Patch on the way. Thanks!

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Sorry for the spam, had to fix a typo.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ah, yes, if you let the events bubble up that could be a problem.  You should still be able to look at event.originalTarget (or whatever it's called) though.

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/12578

Pointer to Github pull-request

Comment 17

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo, please) from comment #12)
> And of course you are right :). The problem is actually with how the trusted
> UI listens to mozbrowser* events. We should be listening to the mozbrowser
> events fired at the iframe embedded within the trusted UI, not at the
> trusted UI iframe itself.

Aha! And as you say in Comment 8, we probably need to change the way we listen to mozbrowsererror in Bug 903134.  I will update that patch.  Thanks, ferjm and khuey!

Comment 18

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

Comment on attachment 812629 [details] [diff] [review]
v1

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

Let's fix this small nit and r? me again once it is done.

::: apps/system/js/trusted_ui.js
@@ +217,5 @@
> +    // Add mozbrowser listeners.
> +    frame.addEventListener('mozbrowserloadstart',
> +                           this.handleBrowserEvent.bind(this));
> +    frame.addEventListener('mozbrowserloadend',
> +                           this.handleBrowserEvent.bind(this));

When you make a call to .bind() it creates a new function. So your event handlers will not be tied directly to this.handleBrowserEvent but to a different function and so calling frame.removeEventListener('...', this.handleBrowserEvent) won't work.

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Thanks Vivien! I think this address your previous comment.

Comment 20

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

Comment on attachment 813120 [details] [diff] [review]
v2

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

r+ with the comment fixed.

::: apps/system/js/trusted_ui.js
@@ +274,5 @@
> +        this.container.removeChild(frame);
> +        frame.removeEventListener('mozbrowserloadstart',
> +                                  this.handleBrowserEvent);
> +        frame.removeEventListener('mozbrowserloadend',
> +                                  this.handleBrowserEvent);

For sanity I would move the this.container.removeChild(frame) below the removeEventListener calls. I'm not 100% sure it will work otherwise due to the nature of mozbrowser.

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Thanks Vivien!

https://github.com/mozilla-b2g/gaia/commit/21d874e03aaa26457ebbda6a06448ea88095b6b5