1174458 - Add WebChannel support to mobile/android

Status: RESOLVED FIXED

(Android Background Services Graveyard :: Firefox Accounts, defect)

Description

[Nick Alexander :nalexander [he/him]]

Bug 1022064 added the WebChannel abstraction to browser/.  This ticket tracks adding the same support to mobile/android/.

Comment 1

[Nick Alexander :nalexander [he/him]]

Testing may be tricky.  If we could enable the existing browser/ mochitests, that would be great.  mobile/android doesn't use the same JS, so I don't think the existing xpcshell tests will be helpful, but others with more knowledge can comment.

vladikoff: MattN: markh: ckarlof: see above, and I wanted to loop you into this.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN

Bug 1174458 - Part 1: Copy-paste WebChannel message support from Desktop. r?MattN

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #2)
> Created attachment 8622005 [details]
> MozReview Request: Bug 1174458 - Part 1: Copy-paste WebChannel message
> support from Desktop. r?MattN
> 
> Bug 1174458 - Part 1: Copy-paste WebChannel message support from Desktop.
> r?MattN

MattN, vladikoff, others: this seems like the little bit of integration glue I need to write a WebChannel-consuming add-on.  (It works fine locally.)  Are there particular security considerations that need verification in Fennec?

Also, I'd like to understand if this glue code could be simplified and shared between the two applications.  Perhaps we can lift more into toolkit/?  Guidance appreciated.

Comment 4

[Matthew N. [:MattN]]

(In reply to Nick Alexander :nalexander from comment #1)
> Testing may be tricky.  If we could enable the existing browser/ mochitests,
> that would be great.  mobile/android doesn't use the same JS, so I don't
> think the existing xpcshell tests will be helpful, but others with more
> knowledge can comment.

Generally the xpcshell tests work cross-platform but I'm not sure which JS you're saying is different? One of the JSMs?

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Matthew N. [:MattN] from comment #4)
> (In reply to Nick Alexander :nalexander from comment #1)
> > Testing may be tricky.  If we could enable the existing browser/ mochitests,
> > that would be great.  mobile/android doesn't use the same JS, so I don't
> > think the existing xpcshell tests will be helpful, but others with more
> > knowledge can comment.
> 
> Generally the xpcshell tests work cross-platform but I'm not sure which JS
> you're saying is different? One of the JSMs?

Hmm, I think I mis-interpreted.  I don't expect https://hg.mozilla.org/integration/fx-team/diff/bf5985c067af/services/fxaccounts/FxAccountsOAuthClient.jsm#l1.20 to do well in Fennec, since the FxA code doesn't work (shouldn't work!) in Fennec.  I thought the xpcshell tests were testing that code, but in fact they are testing the WebChannel.jsm broker.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8622005 [details]
MozReview Request: Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN

https://reviewboard.mozilla.org/r/11107/#review9703

(In reply to Nick Alexander :nalexander from comment #3)
> MattN, vladikoff, others: this seems like the little bit of integration glue
> I need to write a WebChannel-consuming add-on.  (It works fine locally.) 
> Are there particular security considerations that need verification in
> Fennec?

A few things to check off the top of my head:
* Make sure the permission manager works the same as desktop (I think it's shared code).
* Check that the correct principal is sent with messages
* Make sure subframes are handled properly

> Also, I'd like to understand if this glue code could be simplified and
> shared between the two applications.  Perhaps we can lift more into
> toolkit/?  Guidance appreciated.

Yes please. I think toolkit/content/browser-content.js or toolkit/content/process-content.js are good candidates for this code but I'm not sure if those are currently loaded on Android. Loading one of them on Android probably is straightforward.

Can you try to do that and see if it's straightforward?

Could you also try enabling the tests in toolkit/modules/tests/xpcshell/xpcshell.ini?

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Matthew N. [:MattN] from comment #6)
> Comment on attachment 8622005 [details]
> MozReview Request: Bug 1174458 - Part 1: Copy-paste WebChannel message
> support from Desktop. r?MattN
> 
> https://reviewboard.mozilla.org/r/11107/#review9703
> 
> (In reply to Nick Alexander :nalexander from comment #3)
> > MattN, vladikoff, others: this seems like the little bit of integration glue
> > I need to write a WebChannel-consuming add-on.  (It works fine locally.) 
> > Are there particular security considerations that need verification in
> > Fennec?
> 
> A few things to check off the top of my head:
> * Make sure the permission manager works the same as desktop (I think it's
> shared code).
> * Check that the correct principal is sent with messages
> * Make sure subframes are handled properly
> 
> > Also, I'd like to understand if this glue code could be simplified and
> > shared between the two applications.  Perhaps we can lift more into
> > toolkit/?  Guidance appreciated.
> 
> Yes please. I think toolkit/content/browser-content.js or
> toolkit/content/process-content.js are good candidates for this code but I'm
> not sure if those are currently loaded on Android. Loading one of them on
> Android probably is straightforward.
> 
> Can you try to do that and see if it's straightforward?

browser-content.js is definitely loaded in Fennec.

> Could you also try enabling the tests in
> toolkit/modules/tests/xpcshell/xpcshell.ini?

Looking at http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/fx-team-android-api-11/1434403161/fx-team_ubuntu64_vm_armv7_mobile_test-xpcshell-3-bm118-tests1-linux64-build55.txt.gz, these tests are already running on mobile/android \o/

I'll try to make this happen.

Comment 8

[Matthew N. [:MattN]]

(In reply to Nick Alexander :nalexander from comment #7)
> (In reply to Matthew N. [:MattN] from comment #6)
> > Could you also try enabling the tests in
> > toolkit/modules/tests/xpcshell/xpcshell.ini?
> 
> Looking at
> http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/fx-team-
> android-api-11/1434403161/fx-team_ubuntu64_vm_armv7_mobile_test-xpcshell-3-
> bm118-tests1-linux64-build55.txt.gz, these tests are already running on
> mobile/android \o/

I don't regularly have to deal with `toolkit` vs. `os` in test manifests but I saw this in the file:
> skip-if = toolkit == 'android'

Comment 9

[Chris Karlof [:ckarlof]]

stomlinson has been working on this most recently in Desktop. He'd be a good person to include here.

Comment 10

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3343e8d5f0fd

Comment 11

[Vlad Filippov (:vladikoff)]

https://reviewboard.mozilla.org/r/11105/#review10679

::: browser/base/content/content.js:314
(Diff revision 1)
> +// This should be kept in sync with /mobile/android/chrome/content/content.js. 

extra space after comment

::: browser/base/content/content.js:317
(Diff revision 1)
>    // if target is window then we want the document principal, otherwise fallback to target itself.

Nick could you fix / update this comment in both file. i.e it should be: 
`use nodePrincipal` if available, otherwise fallback to document` ?

::: mobile/android/chrome/content/content.js:109
(Diff revision 1)
> +// An event listener for custom "WebChannelMessageToChrome" events on pages.

I think we have tests for this in Desktop to make sure this event works correctly. Do we need to add new tests in Fennec or somehow reuse the existing tests?

Comment 12

[Vlad Filippov (:vladikoff)]

I did my first review via mozreview, apologies if I submit it wrong or something.

Anyway, this looks pretty in sync with Desktop, adding tests that run for Fennec builds and test this event would be reassuring :)

Comment 13

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #10)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3343e8d5f0fd

Worth noting that this try build has many 2.3 rc4 and 4.0 rc9 runs to verify testWebChannel, a Robocop test that duplicates the desktop browser-chrome test_web_channel.js.

Comment 14

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=957a2ecafcce

Comment 15

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1644eb1c67c3

Comment 16

[Shane Tomlinson [:stomlinson]]

Consider me notified, removing the needinfo request.

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #15)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1644eb1c67c3

In api 11 opt X (http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/[e-mail]-8a6645645212/try-android-api-11/try_panda_android_test-xpcshell-bm102-tests1-panda-build2160.txt.gz), I see:

23:18:51     INFO -  TEST-START | toolkit/modules/tests/xpcshell/test_web_channel.js
23:18:52     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_web_channel.js | took 782ms
23:18:52     INFO -  mozdevice getting files in '/mnt/sdcard/tests/xpcshell/minidumps'
23:18:52     INFO -  TEST-START | toolkit/modules/tests/xpcshell/test_web_channel_broker.js
23:18:53     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_web_channel_broker.js | took 789ms
23:18:53     INFO -  mozdevice getting files in '/mnt/sdcard/tests/xpcshell/minidumps'

So it looks like we're running (and passing!).  But I don't understand why the minidumps are being fetched -- is this just noise?  gbrown, are these tests actually crashing?

Comment 18

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8622005 [details]
MozReview Request: Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN

Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN

This makes WebChannel support available to all XUL applications that
use toolkit/, including browser/ and mobile/android/.

The new Robocop tests are necessary because we can't run the existing
browser-chrome tests on Android (yet).

Comment 19

[Geoff Brown [:gbrown]]

Just noise - no problem/crash. (We try to get files from minidumps simply to determine if there has been a crash. I'm not sure why, with all the log info we suppress, this message is displayed, but it is nothing to worry about.)

Comment 20

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN

Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN

Comment 21

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 964854 - Reveal cause of exception. r=rnewman

Bug 964854 - Reveal cause of exception. r=rnewman

Comment 22

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 964854 - PII debug logging.

Bug 964854 - PII debug logging.

Comment 23

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1177855: Fetch and show avatar image as preference icon -r=nalexander.

Bug 1177855: Fetch and show avatar image as preference icon -r=nalexander.

Comment 24

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1177855 : Miscellaneous improvements related to profile avatar caching -r=nalexander.

Bug 1177855 : Miscellaneous improvements related to profile avatar caching -r=nalexander.

Comment 25

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1177855 - simplified avatar fetch logic r?nalexander.

Bug 1177855 - simplified avatar fetch logic r?nalexander.

* Generalized PicassoPreferenceIcon implementation

* Cleaned up profile caching and simplified the complex profile fetch rate limiter logic

* profile always de-serialized from account manager rather from a cache.

* UI always updated asynchronously through broadcast after sync. A defaul icon is used as
  fallback during the on going sync.

Comment 26

[Nick Alexander :nalexander [he/him]]

Lots of commits hit the wrong ticket.  Sorry for the noise, folks.

Comment 27

[Richard Newman [:rnewman]]

Comment on attachment 8627923 [details]
MozReview Request: Bug 964854 - Reveal cause of exception. r=rnewman

https://reviewboard.mozilla.org/r/12329/#review10795

Ship It!

Comment 28

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8622005 [details]
MozReview Request: Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN

MattN is on PTO.  markh, can you look at this?

Comment 29

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8622005 [details]
MozReview Request: Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN

https://reviewboard.mozilla.org/r/11107/#review11047

Ship It!

Comment 30

[Nick Alexander :nalexander [he/him]]

url:        https://hg.mozilla.org/integration/fx-team/rev/419e99a6f242f4d64c9085b150aee1c182294a8b
changeset:  419e99a6f242f4d64c9085b150aee1c182294a8b
user:       Nick Alexander <[e-mail]>
date:       Tue Jun 30 11:46:27 2015 -0700
description:
Bug 1174458 - Move WebChannel message support to toolkit/content. r=markh

This makes WebChannel support available to all XUL applications that
use toolkit/, including browser/ and mobile/android/.

The new Robocop tests are necessary because we can't run the existing
browser-chrome tests on Android (yet).

Comment 31

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/419e99a6f242