1395217 - Use "Child" instead of "Content" to denote GeckoView child process modules

Status: RESOLVED FIXED

(GeckoView :: Sandboxing, defect, P3)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Right now we have GeckoViewContent.js (frame script for child process) and GeckoViewContent.jsm (module for content listener in parent process). I think it'd be less confusing if we rename GeckoViewContent.js to GeckoViewChild.js (and rename GeckoViewContentModule.jsm to GeckoViewChildModule.jsm as well).

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment 0 isn't totally correct. GeckoViewContent.js is the frame script for GeckoViewContent.jsm in the child process. Similarly, GeckoViewScrollContent.js is the frame script for GeckoViewScroll.jsm in the child process.

So I think the correct changes should be,
* Rename GeckoViewContent.js to GeckoViewContentChild.js
* Rename GeckoViewScrollContent.js to GeckoViewScrollChild.js
* Rename GeckoViewContentModule.jsm to GeckoViewChildModule.jsm

These changes will avoid the confusion of using the term "Content" to denote both "child process" and "content listener".

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

All of that sounds good except:

> * Rename GeckoViewContentModule.jsm to GeckoViewChildModule.jsm

That one is named to be consistent with the "content listener" functionality, as you said, so it seems like it should stay as-is. This module may not even interact with a child (content) process.

Comment 3

[Manish [:manishkk]]

Hi. I'd like to be assigned to solve this bug. :)

Do I need to change anything inside any file?

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Thanks! Yes you have to change the corresponding names inside the renamed files. For example, after renaming GeckoViewScrollContent.js to GeckoViewScrollChild.js, you need to change `class GeckoViewScrollContent` [1] to `class GeckoViewScrollChild` inside that file.

[1] https://searchfox.org/mozilla-central/rev/028cd8342899357d80fafba2d528a0fd5819e316/mobile/android/chrome/geckoview/GeckoViewScrollContent.js#17

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #2)
> All of that sounds good except:
> 
> > * Rename GeckoViewContentModule.jsm to GeckoViewChildModule.jsm
> 
> That one is named to be consistent with the "content listener"
> functionality, as you said, so it seems like it should stay as-is. This
> module may not even interact with a child (content) process.

GeckoViewContentModule.jsm is actually used by child process frame scripts, so should be GeckoViewChildModule.jsm. It's confusing :) (hence the need for this bug)

Comment 6

[Manish [:manishkk]]

Attachment: Patch_Bug1395217

Please review and assign this bug to me.

Thanks

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8964186 [details] [diff] [review]
Patch_Bug1395217

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

Looking good Manish! Just a few more changes to make:

1) Rename "GeckoViewNavigationContent.js" to "GeckoViewNavigationChild.js". This file was added recently.

2) Rename "GeckoViewContentSettings.js" to "GeckoViewSettingsChild.js". This file was also added recently.

3) In all the files that you changed, change "GeckoViewContentModule" to "GeckoViewChildModule" as well.

For example, several files have this line
>  ChromeUtils.import("resource://gre/modules/GeckoViewContentModule.jsm");
which should be changed to
>  ChromeUtils.import("resource://gre/modules/GeckoViewChildModule.jsm");

and this line
>  class XXX extends GeckoViewContentModule
which should be changed to
>  class XXX extends GeckoViewChildModule

4) Search for references to the old file names and change them to the new names.

https://searchfox.org is a good way to search for references. For example, if you search for "GeckoViewScrollContent.js", you can see it is referenced in "mobile/android/chrome/geckoview/jar.mn" and "mobile/android/modules/geckoview/GeckoViewScroll.jsm", so those files should be changed to use "GeckoViewScrollChild.js".

Comment 8

[Manish [:manishkk]]

Attachment: Patch_Bug1395217

Please Review.
Thanks

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8964432 [details] [diff] [review]
Patch_Bug1395217

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

Thanks! Almost there! Just a few changes to make.

1) Make sure you change "GeckoViewScrollContent.js" to "GeckoViewScrollChild.js" in mobile/android/modules/geckoview/GeckoViewScroll.jsm

1) Make sure you change "GeckoViewContent.js" to "GeckoViewContentChild.js" in mobile/android/modules/geckoview/GeckoViewScroll.jsm

::: mobile/android/chrome/geckoview/jar.mn
@@ +9,3 @@
>    content/geckoview.xul
>    content/geckoview.js
>    content/GeckoViewContent.js

Change this line to "content/GeckoViewContentChild.js"

@@ +9,4 @@
>    content/geckoview.xul
>    content/geckoview.js
>    content/GeckoViewContent.js
> +  content/GeckoViewSettingsChild.js

Move this line below the last line (content/GeckoViewScrollContent.js), so they're in alphabetical order.

@@ +14,1 @@
>    content/GeckoViewScrollContent.js

Change this line to "content/GeckoViewScrollChild.js"

::: mobile/android/modules/geckoview/moz.build
@@ +7,4 @@
>  EXTRA_JS_MODULES += [
>      'AndroidLog.jsm',
>      'GeckoViewContent.jsm',
> +    'GeckoViewChildModule.jsm',

Move this line above the previous line ('GeckoViewContent.jsm',) so they're in alphabetical order.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #9)

> 1) Make sure you change "GeckoViewContent.js" to "GeckoViewContentChild.js"
> in mobile/android/modules/geckoview/GeckoViewScroll.jsm

Sorry, that should be make sure you change "GeckoViewContent.js" to "GeckoViewContentChild.js" in mobile/android/modules/geckoview/GeckoViewContent.jsm

Comment 11

[Manish [:manishkk]]

How can I test this after changes on my system?

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

You should make sure it builds, and then you can run our automated tests [1] on your machine. Another option is, if you upload a patch, we can help you push a try run that will run the tests on Mozilla's infrastructure.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Geckoview-Junit_Tests

Comment 13

[Emily Toop (:fluffyemily)]

Attachment: geckoview-official-withGeckoBinaries-noMinApi-debug-androidTest.apk

:jchen this is the apk that is causing me build issues mentioned in https://mozilla.slack.com/archives/C8Y4T8UQH/p1538494590000100

Comment 14

[Emily Toop (:fluffyemily)]

Attachment: Bug1395217_Etoop.patch

My first m-c patch so please be gentle :)

Comment 15

[Chris Peterson [:cpeterson]]

Comment on attachment 9014091 [details] [diff] [review]
Bug1395217_Etoop.patch

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

r?jchen

Comment 16

[Chris Peterson [:cpeterson]]

Thanks, Emily! To request a code review, just set the patch's review status flag to "review?" ("r?"). The code reviewer will set the patch's review status flag to "review+" ("r+") to approve a patch.

We might want to uplift this change to GV 63 Beta to make uplifts easier. If someone fixes a bug in 64's GeckoViewChildModule.jsm, uplifting the fix to 63's old filename GeckoViewContentModule.jsm will require some manual patch rebasing.

Comment 17

[Emily Toop (:fluffyemily)]

(In reply to Chris Peterson [:cpeterson] from comment #16)
> Thanks, Emily! To request a code review, just set the patch's review status
> flag to "review?" ("r?"). The code reviewer will set the patch's review
> status flag to "review+" ("r+") to approve a patch.

Oops, sorry. It's been quite a while since I last used bugzilla!

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9014091 [details] [diff] [review]
Bug1395217_Etoop.patch

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

Thanks Emily! Looks good with only a couple changes needed.

::: mobile/android/chrome/geckoview/GeckoViewProgressContent.js
@@ -9,5 @@
>  XPCOMUtils.defineLazyModuleGetters(this, {
>    Services: "resource://gre/modules/Services.jsm",
>  });
>  
> -class GeckoViewProgressContent extends GeckoViewContentModule {

`GeckoViewProgressContent` to `GeckoViewProgressChild` (and rename the file)

::: mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js
@@ +12,4 @@
>  
>  // Dispatches GeckoView:ShowSelectionAction and GeckoView:HideSelectionAction to
>  // the GeckoSession on accessible caret changes.
> +class GeckoViewSelectionActionContent extends GeckoViewChildModule {

`GeckoViewSelectionActionContent` to GeckoViewSelectionActionChild` (and rename the file too)

::: mobile/android/chrome/geckoview/jar.mn
@@ +14,2 @@
>    content/GeckoViewProgressContent.js
>    content/GeckoViewPromptContent.js

Rename "GeckoViewPromptContent.js" to "GeckoViewPromptChild.js"

Comment 19

[Emily Toop (:fluffyemily)]

Attachment: Bug1395217_2.patch

Addressed Review Comments

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9015210 [details] [diff] [review]
Bug1395217_2.patch

r+ assuming tests pass. Thanks Emily!

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

Hi Emily, just a reminder to land this. It's in danger of bit-rotting :)

Comment 22

[Emily Toop (:fluffyemily)]

(In reply to Jim Chen [:jchen] [:darchons] from comment #21)
> Hi Emily, just a reminder to land this. It's in danger of bit-rotting :)

Hi, I've never landed anything in m-c before. What's the best way to do that?

Comment 23

[Chris Peterson [:cpeterson]]

(In reply to Emily Toop (:fluffyemily) from comment #22)
> Hi, I've never landed anything in m-c before. What's the best way to do that?

1. The easiest option is to add the "checkin-needed" Bugzilla keyword to the bug. I'll do that now. :)

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_6_Getting_code_into_the_tree

2. The second easiest option is to use Phabricator code review tool (instead of attaching raw patch files) and its auto-lander bot, Lando:

https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#quick-start

3. The third option is push your patch to the mozilla-inbound Mercurial server.

https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/index.html

* For options 2 and 3, you will need Mozilla commit access level 3 to push code to Mercurial:

https://www.mozilla.org/en-US/about/governance/policies/commit/

Comment 24

[Eliza Balazs [:ebalazs_]]

Emily: Hi, we received the following errors when trying to apply your patch, so please provide an updated one.

applying Bug1395217_2.patch
10:53 AM patching file mobile/android/chrome/geckoview/GeckoViewProgressChild.js
10:53 AM Hunk #1 FAILED at 9
10:53 AM 1 out of 2 hunks FAILED -- saving rejects to file mobile/android/chrome/geckoview/GeckoViewProgressChild.js.rej
10:53 AM patching file mobile/android/chrome/geckoview/GeckoViewSelectionActionChild.js
10:53 AM Hunk #1 FAILED at 11
10:53 AM 1 out of 2 hunks FAILED -- saving rejects to file mobile/android/chrome/geckoview/GeckoViewSelectionActionChild.js.rej
10:53 AM patching file mobile/android/chrome/geckoview/jar.mn
10:53 AM Hunk #1 FAILED at 10
10:53 AM 1 out of 1 hunks FAILED -- saving rejects to file mobile/android/chrome/geckoview/jar.mn.rej
10:53 AM unable to find 'mobile/android/chrome/geckoview/GeckoViewProgressContent.js' for patching
10:53 AM (use '--prefix' to apply patch relative to the current directory)
10:53 AM 1 out of 1 hunks FAILED -- saving rejects to file mobile/android/chrome/geckoview/GeckoViewProgressContent.js.rej
10:53 AM unable to find 'mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js' for patching
10:53 AM (use '--prefix' to apply patch relative to the current directory)
10:53 AM 2 out of 2 hunks FAILED -- saving rejects to file mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js.rej
10:53 AM patching file mobile/android/modules/geckoview/GeckoViewChildModule.jsm
10:53 AM Hunk #1 FAILED at 3
10:53 AM 1 out of 1 hunks FAILED -- saving rejects to file mobile/android/modules/geckoview/GeckoViewChildModule.jsm.rej
10:53 AM patch failed, unable to continue (try -v)
10:53 AM patch failed, rejects left in working directory
10:53 AM errors during apply, please fix and qrefresh Bug1395217_2.patch

Clearing the checkin-needed. Thanks!

Comment 25

[Emily Toop (:fluffyemily)]

Attachment: Bug1395217_Rebased.patch

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Please provide a patch which successfully applies with |hg import|.

Comment 27

[Emily Toop (:fluffyemily)]

Attachment: bug-1395217_rebased.patch

I really hope this works. It's been a real headache figuring out how to rebase on top of mercurial. This applied successfully locally.

Comment 28

[Raul Gurzau (:RaulG)]

Encountered this while applying the patch:

applying bug-1395217_rebased.patch
patching file mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
Hunk #1 FAILED at 562
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug-1395217_rebased.patch

Removing checkin-needed tag.

Comment 29

[Emily Toop (:fluffyemily)]

Attachment: Bug 1395217 - Rename Use "Child" instead of "Content" to denote GeckoView child process modules

Renamed:

* `GeckoViewContent.js` -> `GeckoViewContentChild.js`
* `GeckoViewNavigationContent.js` -> `GeckViewNavigationChild.js`
* `GeckoViewProgressContent.js` -> `GeckoViewProgressChild.js`
* `GeckoViewPromptContent.js` -> `GeckoViewPromptChild.js`
* `GeckoViewScrollContent.js` -> `GeckoViewScrollChild.js`
* `GeckoViewSelectionActionContent.js` -> `GeckoViewSelectionActionChild.js`
* `GeckoViewSettingsContent.js` -> `GeckoViewSettingsChild.js`
* `GeckoViewContentModule.jsm` -> `GeckoViewChildModule.jsm`

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7331c7194e20
Rename Use "Child" instead of "Content" to denote GeckoView child process modules r=snorp

Comment 31

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/7331c7194e20

Comment 32

[Chris Peterson [:cpeterson]]

64=wontfix because we don't need to uplift this fix for Focus 8.0's GV 64 beta test

Comment 33

[Chris Peterson [:cpeterson]]

status-geckoview64=wontfix

Comment 34

[Chris Peterson [:cpeterson]]

Moving content process bugs to the new GeckoView::Sandboxing component.