1154277 - Introduce a new pref for Loop e10s work

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mike de Boer [:mikedeboer]]

Since the work on enabling e10s for Loop will be spread out across several bugs, we'll need to put this functionality behind a flag.

This means that the `remote=true` flag on the browser element will be dependent on this pref's value.

Suggested name: `loop.remote.enabled = false` (`false` by default).

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: WIP: run Loop Conversation in the content process

Hi Shane! I'd really like to hear your opinion on this WIP patch. It makes a chat window document run in the content process (so the browser becomes a remote browser).
What do you think of the messageManager parts here? It'll work for non-e10s chat documents too.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8689072 [details] [diff] [review]
WIP: run Loop Conversation in the content process

Generally looks fine.

Can we skip Social: on standard events (e.g. DOMWindowClose instead of Social:DOMWindowClose)?

I don't see mm being used to handle DOMContentLoaded anywhere else, are you sure that it necessary?

Should stuff like Social:GetAllWebrtcStats be Loop:GetAllWebrtcStats?  If it is not in the shared js, I'd go for Loop:.

If you wanted to, you could replace use of socialFrameShow/Hide with the standard visibility API.

Comment 3

[Mike de Boer [:mikedeboer]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Comment on attachment 8689072 [details] [diff] [review]
> WIP: run Loop Conversation in the content process
> 
> Generally looks fine.
> 
> Can we skip Social: on standard events (e.g. DOMWindowClose instead of
> Social:DOMWindowClose)?
> 
> I don't see mm being used to handle DOMContentLoaded anywhere else, are you
> sure that it necessary?
> 
> Should stuff like Social:GetAllWebrtcStats be Loop:GetAllWebrtcStats?  If it
> is not in the shared js, I'd go for Loop:.
> 
> If you wanted to, you could replace use of socialFrameShow/Hide with the
> standard visibility API.

Thanks for advice! And see you in Orlando soon ;-)

Comment 4

[Mark Banner (:standard8)]

I just had a look at this patch with respect to the system add-on (bug 1186172), as I was worried about some build flags being necessary.

According to https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Which_URIs_load_where there's various flags which affect the behaviour of where things load. Currently those checks are only done in tabbrowser.xml.

I don't think we need to add the checks here, but I think it might be an idea to update AboutRedirector to specify URI_CAN_LOAD_IN_CHILD or URI_MUST_LOAD_IN_CHILD in case someone is looking through later and gets confused.

It might also be worth a comment or two in that file, to say where the remote browser is currently specified.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: rebased version of remotelogic, with linux/platform.css added WIP

also I commented out the 'don't use e10s' stuff

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: disable TCP TURN (NOT FOR CHECKIN)

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

The patch in bug 1226200 will let this patch be landable, it appears

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: introduce a pref to switch e10s support for Loop/ Hello on or off

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: make chat windows run in the content process

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8701499 [details] [diff] [review]
Patch 1: introduce a pref to switch e10s support for Loop/ Hello on or off

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

Looks good. r=Standard8

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8701503 [details] [diff] [review]
Patch 2: make chat windows run in the content process

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

I haven't tested it, but the general ideas here look fine.

::: browser/extensions/loop/bootstrap.js
@@ +136,5 @@
> +            let mm = iframe.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader.messageManager;
> +
> +            mm.sendAsyncMessage("Social:WaitForDocumentVisible");
> +            mm.addMessageListener("Social:DocumentVisible", () => {
> +              if (tabId) {

We could drop the tabId stuff whilst we're here - we no longer do it. I've been meaning to do it at some stage and keep forgetting.

::: dom/base/nsFrameLoader.cpp
@@ +929,5 @@
>    nsCOMPtr<nsIBrowserDOMWindow> browserDOMWindow =
>      mRemoteBrowser->GetBrowserDOMWindow();
>  
> +  if (otherBrowserDOMWindow && browserDOMWindow) {
> +    aOther->mRemoteBrowser->SetBrowserDOMWindow(browserDOMWindow);

Just to note, this will need reviewing by a dom peer. I don't mind reviewing the rest though.

::: toolkit/content/widgets/browser.xml
@@ +1240,5 @@
>              // Rewire the remote listeners
>              this._remoteWebNavigation.swapBrowser(this);
>              aOtherBrowser._remoteWebNavigation.swapBrowser(aOtherBrowser);
>  
> +            if (this._remoteWebProgressManager && aOtherBrowser._remoteWebProgressManager) {

Should we be providing some sort of warning if these aren't the same?

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: allow to swap docShells on remote browsers that are not a child of a DOMWindow

Olli, this change is necessary, because <browser> elements inside a Chat window are not attached to a DOMWindow, so there's nothing to swap in that case.

Comment 13

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3: support running Social API documents to run in a remote browser

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4: make chat windows run in the content process and re-enable browser sharing

Separated this out so it can be applied to the add-on later as well.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8705115 [details] [diff] [review]
Patch 2.1: allow to swap docShells on remote browsers that are not a child of a DOMWindow

>Bug 1154277: Part 2 - allow to swap docShells on remote browsers that are not a child of a DOMWindow. r?smaug
This is not about DOMWindow, but about BrowserDOMWindow, so fix the commit message.


>-  if (!otherBrowserDOMWindow || !browserDOMWindow) {
>-    return NS_ERROR_NOT_IMPLEMENTED;
Could you leave this if-check but change it to
if (!!otherBrowserDOMWindow != !!browserDOMWindow) {
  return NS_ERROR_NOT_IMPLEMENTED;
}

>+  if (otherBrowserDOMWindow && browserDOMWindow) {
>+    aOther->mRemoteBrowser->SetBrowserDOMWindow(browserDOMWindow);
>+    mRemoteBrowser->SetBrowserDOMWindow(otherBrowserDOMWindow);
>   }
And then you wouldn't actually need this change. We'd just replace null BrowserDOMWindow with null two lines below.

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.2: allow to swap docShells on remote browsers that are not a child of a BrowserDOMWindow

Carrying over r=smaug. Thanks Olli!

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.1: make chat windows run in the content process and re-enable browser sharing

Updated panel height setting to not happen in chrome anymore.

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.1: support running Social API documents to run in a remote browser

Moved test updated over from patch 4 to here.

Comment 19

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.2: make chat windows run in the content process and re-enable browser sharing

Moved test updates over to part 3.

Comment 20

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8706341 [details] [diff] [review]
Patch 3.1: support running Social API documents to run in a remote browser

- if loop is an addon, shouldn't loop code go into the addon?  I understand the convenience of putting the loop code here, but it does break the purpose of putting loop into an addon.
- the toolkit looks fine to me, though I wonder why this is necessary for social frames

Comment 21

[Mike de Boer [:mikedeboer]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> - if loop is an addon, shouldn't loop code go into the addon?  I understand
> the convenience of putting the loop code here, but it does break the purpose
> of putting loop into an addon.
> - the toolkit looks fine to me, though I wonder why this is necessary for
> social frames

Thanks for the feedback!

The reason is quite simple and unfortunate: the 'remote' attribute has to be set in the binding (i.e. not dynamically in the constructor or elsewhere), because otherwise bad things happen and it'll not show we throw at it.
Removing the remote attribute dynamically as soon as we logically can, however, does seem to work.

If it weren't for these 'quirks', I would've put it all inside the add-on... but helas!

Comment 22

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.3: make chat windows run in the content process and re-enable browser sharing

Comment 23

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: introduce a pref to switch e10s support for Loop/ Hello on or off

Rebased patch. Carrying over r=Standard8.

Comment 24

[Shane Caraveo (:mixedpuppy)]

(In reply to Mike de Boer [:mikedeboer] from comment #21)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> > - if loop is an addon, shouldn't loop code go into the addon?  I understand
> > the convenience of putting the loop code here, but it does break the purpose
> > of putting loop into an addon.
> > - the toolkit looks fine to me, though I wonder why this is necessary for
> > social frames
> 
> Thanks for the feedback!
> 
> The reason is quite simple and unfortunate: the 'remote' attribute has to be
> set in the binding (i.e. not dynamically in the constructor or elsewhere),
> because otherwise bad things happen and it'll not show we throw at it.
> Removing the remote attribute dynamically as soon as we logically can,
> however, does seem to work.
> 
> If it weren't for these 'quirks', I would've put it all inside the add-on...
> but helas!

I wasn't looking at the remote attr, having remote browsers should be generally helpful.  There is loop specific stuff in social-content.js and PanelFrame.jsm.  A content script should work just fine from an addon, and PanelFrame could be reworked to allow additions (e.g. showPopup grows an attrs attr).

Comment 25

[Mike de Boer [:mikedeboer]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #24)
> (In reply to Mike de Boer [:mikedeboer] from comment #21)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> > > - if loop is an addon, shouldn't loop code go into the addon?  I understand
> > > the convenience of putting the loop code here, but it does break the purpose
> > > of putting loop into an addon.
> > > - the toolkit looks fine to me, though I wonder why this is necessary for
> > > social frames
> > 
> > Thanks for the feedback!
> > 
> > The reason is quite simple and unfortunate: the 'remote' attribute has to be
> > set in the binding (i.e. not dynamically in the constructor or elsewhere),
> > because otherwise bad things happen and it'll not show we throw at it.
> > Removing the remote attribute dynamically as soon as we logically can,
> > however, does seem to work.
> > 
> > If it weren't for these 'quirks', I would've put it all inside the add-on...
> > but helas!
> 
> I wasn't looking at the remote attr, having remote browsers should be
> generally helpful.  There is loop specific stuff in social-content.js and
> PanelFrame.jsm.  A content script should work just fine from an addon, and
> PanelFrame could be reworked to allow additions (e.g. showPopup grows an
> attrs attr).

Ah ok. Sure, can do. Or I can do that in a follow-up that's less time-critical.

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.2: support running Social API documents to run in a remote browser

Test fixups.

Comment 27

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.4: make chat windows run in the content process and re-enable browser sharing

Test fixes.

Comment 28

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: introduce a pref to switch e10s support for Loop/ Hello on or off

Comment 29

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: introduce a pref to switch e10s support for Loop/ Hello on or off

Comment 30

[Mike de Boer [:mikedeboer]]

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

Comment 31

[Shane Caraveo (:mixedpuppy)]

> I can do that in a follow-up that's less
> time-critical.

+1

Comment 32

[Mark Banner (:standard8)]

With the latest patches, I'm seeing this when I pop-out the window. It appears to create the window and not finish destroying the old.

NS_ERROR_NOT_IMPLEMENTED at browser.xml line 1209:

         this.QueryInterface(Components.interfaces.nsIFrameLoaderOwner)
              .swapFrameLoaders(aOtherBrowser);

Comment 33

[Mark Banner (:standard8)]

Comment on attachment 8707503 [details] [diff] [review]
Patch 3.2: support running Social API documents to run in a remote browser

Clearing to wait for follow-ups.

Comment 34

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f568b332554

Comment 35

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=355d6be945c9

Comment 36

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9136bed24760

Comment 37

[Mike de Boer [:mikedeboer]]

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

Comment 38

[Mike de Boer [:mikedeboer]]

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

Comment 39

[GitHub Autolander Bot]

Attachment: [loop] mikedeboer:e10s > mozilla:master

Comment 40

[Mike de Boer [:mikedeboer]]

Comment on attachment 8712136 [details] [review]
[loop] mikedeboer:e10s > mozilla:master

This PR doesn't work without the updated browser patches. But certainly ready.

Comment 41

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.3: support running Social API documents to run in a remote browser

Comment 42

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4: implement test changes to work in e10s mode as well

Comment 43

[Mike de Boer [:mikedeboer]]

Comment on attachment 8712711 [details] [diff] [review]
Patch 4: implement test changes to work in e10s mode as well

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

::: browser/base/content/test/chat/browser_focus.js
@@ +11,5 @@
>  function isTabFocused() {
>    let tabb = gBrowser.getBrowserForTab(gBrowser.selectedTab);
>    // focus sucks in tests - our window may have lost focus.
>    let elt = Services.focus.getFocusedElementForWindow(window, false, {});
> +  dump("WHATS THE EL??\nA:" + elt.parentNode.outerHTML + "\nB:" + tabb.parentNode.outerHTML + "\n");

I'll be removing this...

Comment 44

[Mike de Boer [:mikedeboer]]

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

Comment 45

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8712711 [details] [diff] [review]
Patch 4: implement test changes to work in e10s mode as well

Needs a bug # for the crash comment.  Remove dumps.

Comment 46

[Mike de Boer [:mikedeboer]]

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

Comment 47

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.4: support running Social API documents to run in a remote browser

Comment 48

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.1: implement test changes to work in e10s mode as well

Carrying over r=mixedpuppy - thanks Shane!!

Comment 49

[Mike de Boer [:mikedeboer]]

Note-to-try-watchers: browser_mozLoop_sharingListeners.js is failing due to the about:newtab shenanigans. You may ignore this, since the upcoming Loop code uplift should work with this. At least, it's unrelated to the work going on in this bug.

Comment 50

[Mike de Boer [:mikedeboer]]

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

Comment 51

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=424dfe8a7896

Comment 52

[Mike de Boer [:mikedeboer]]

^-- says 'all green' once I disable 'browser/base/content/test/social/browser_social_status.js' (temporarily). That test runs fine for me locally, so needs a bit of investigation beyond this bug.

Comment 53

[Mark Banner (:standard8)]

Comment on attachment 8713140 [details] [diff] [review]
Patch 3.4: support running Social API documents to run in a remote browser

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

r=me once you've addressed the comments, unless that remote attribute issue causes bigger rework.

::: browser/base/content/socialchat.xml
@@ +584,5 @@
>          ]]></body>
>        </method>
>  
>        <method name="openChat">
> +        <parameter name="aOptions"/>

I think you really want to document the possible options here - a couple of lines below, you extract most things, but not the remote. So unless people examine the code, they aren't going to pick up on all the possible options.

@@ +737,5 @@
>              let otherChatbox = otherWin.document.getElementById("chatter");
>              aChatbox.setDecorationAttributes(otherChatbox);
>              aChatbox.swapDocShells(otherChatbox);
> +            if (!aChatbox.content.hasAttribute("remote")) {
> +              otherChatbox.content.removeAttribute("remote");

This feels like the wrong order. If the aChatbox isn't remote, and otherChatbox is, then I can see swapDocShells having issues.

I don't think it'll affect the crash you're seeing at the moment, but I suspect it might cause extra issues for detaching in non-e10s mode, as the remote attribute is default.

Comment 54

[Mark Banner (:standard8)]

Comment on attachment 8712136 [details] [review]
[loop] mikedeboer:e10s > mozilla:master

I think this is almost there, I'd just like to have a look at the changes once you've addressed the comments.

Comment 55

[Mike de Boer [:mikedeboer]]

(In reply to Mark Banner (:standard8) from comment #53)
> @@ +737,5 @@
> >              let otherChatbox = otherWin.document.getElementById("chatter");
> >              aChatbox.setDecorationAttributes(otherChatbox);
> >              aChatbox.swapDocShells(otherChatbox);
> > +            if (!aChatbox.content.hasAttribute("remote")) {
> > +              otherChatbox.content.removeAttribute("remote");
> 
> This feels like the wrong order. If the aChatbox isn't remote, and
> otherChatbox is, then I can see swapDocShells having issues.
> 
> I don't think it'll affect the crash you're seeing at the moment, but I
> suspect it might cause extra issues for detaching in non-e10s mode, as the
> remote attribute is default.

Well, I agree that is seems the wrong order, but all this has to do with the fact that we can't set the 'remote' attribute at a later time. Hence I had to put it there in the binding and remove it explicitly as soon as we know that we're not in remote mode.
What I can look into, however, is to defer setting the 'src' until after we're sure that remote is necessary.

Comment 56

[Mike de Boer [:mikedeboer]]

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

Comment 57

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d91ecb274a9

Comment 58

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.5: support running Social API documents to run in a remote browser

Carrying over r=Standard8, mixedpuppy.

So the difference I made here is to have two <xul:browser> elements in the chatbox binding - one remote and one not remote. To make sure that the document isn't loaded twice, I explicitly set the src to 'about:blank' in the other browser instance that won't be used.
This would potentially allow runtime switching between remote and non-remote, but that won't happen in practice.

I also made sure that detaching a chat window works in non-e10s browser windows. Doing this in e10s browsers will crash the content process, currently.

There's another issue with messageManager instance management wrt `swapDocShells` - the messageManager gets detached somehow and RemotePageManager.jsm messages are lost in the void.

At this point there's so much brokenness with e10s on that I'm advising to drop support for detaching a window altogether for now, until things stabilize.

Comment 59

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.2: implement test changes to work in e10s mode as well

Carrying over r=Standard8.

Comment 60

[Mike de Boer [:mikedeboer]]

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

Comment 61

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ea89adf35aa

Comment 62

[Mike de Boer [:mikedeboer]]

The try push in comment 61 deems this ready to land. Mark, are we ready to do an add-on push at the same time as pushing this to the tree?

Comment 63

[Mark Banner (:standard8)]

As discussed on irc, I've got an L10n patch in flight (bug 1239828) which will hopefully get r+ overnight. So we'll try and land this tomorrow.

Comment 64

[Mark Banner (:standard8)]

Comment on attachment 8712136 [details] [review]
[loop] mikedeboer:e10s > mozilla:master

r=Standard8 once you've give the XXX in the browser.ini a bug number.

Comment 65

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/30006ad754bf44b906de8fb9ed2b6236caf43406
Bug 1154277: Part 1 - allow to swap docShells on remote browsers that are not a child of a BrowserDOMWindow. r=smaug

https://hg.mozilla.org/integration/fx-team/rev/a4ede73cb8b3f7d87fbd83976406e174749a76a8
Bug 1154277: Part 2 - support running Social API documents to run in a remote browser, i.e. the content process. f=mixedpuppy, r=Standard8

https://hg.mozilla.org/integration/fx-team/rev/da59790ccdad8a053591348b84adeb2d74c25400
Bug 1154277: Part 3 - implement test changes to work in e10s mode as well. r=mixedpuppy,Standard8

https://hg.mozilla.org/integration/fx-team/rev/6371c0aebc73a3b8f049625cc93571120969fc30
Bug 1154277: Part 4 - Update the loop add-on and introduce a pref to switch e10s support for Loop/ Hello on or off. The default for now is off. r=Standard8

Comment 66

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=6371c0aebc73

Comment 67

[:Paolo Amadini]

ESLint failures?

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=6371c0aebc73

Comment 68

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/d2522fbd0f6328c7c2b54975636d951924ec7372
Bug 1154277: follow-up - fix linting error by removing a leftover break statement. rs=me

Comment 69

[GitHub Autolander Bot]

Attachment: [loop] mikedeboer:e10s-followup > mozilla:master

Comment 70

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/30006ad754bf
https://hg.mozilla.org/mozilla-central/rev/a4ede73cb8b3
https://hg.mozilla.org/mozilla-central/rev/da59790ccdad
https://hg.mozilla.org/mozilla-central/rev/6371c0aebc73
https://hg.mozilla.org/mozilla-central/rev/d2522fbd0f63

Comment 71

[Mike de Boer [:mikedeboer]]

Comment on attachment 8705191 [details] [diff] [review]
Patch 2.2: allow to swap docShells on remote browsers that are not a child of a BrowserDOMWindow

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: When e10s is enabled by default, Loop will not start. This patch is one of three necessary to get things running.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, tests pass. Chat tests needed to be disabled, but not signal of functionality loss.
[Risks and why]: This patch has minor risk. Other patches would be higher.
[String/UUID change made/needed]: n/a.

Comment 72

[Mike de Boer [:mikedeboer]]

Comment on attachment 8714806 [details] [diff] [review]
Patch 3.5: support running Social API documents to run in a remote browser

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: When e10s is enabled by default, Loop will not start. This patch is one of three necessary to get things running.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, tests pass. Chat tests needed to be disabled, but not signal of functionality loss.
[Risks and why]: Moderate risk, due to disabled tests. Possible bitrot would pose minimal risk; relatively easy to fix.
[String/UUID change made/needed]: n/a.

Comment 73

[Mike de Boer [:mikedeboer]]

Comment on attachment 8714807 [details] [diff] [review]
Patch 4.2: implement test changes to work in e10s mode as well

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: When e10s is enabled by default, Loop will not start. This patch is one of three necessary to get things running.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, tests pass. Chat tests needed to be disabled, but not signal of functionality loss.
[Risks and why]: Moderate risk, due to disabled test. Possible bitrot would pose minimal risk; relatively easy to fix.
[String/UUID change made/needed]: n/a.

Comment 74

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8714806 [details] [diff] [review]
Patch 3.5: support running Social API documents to run in a remote browser

Workaround to get Hello as a system addon working when e10s is enabled. 
OK to uplift to aurora.

Comment 75

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8714807 [details] [diff] [review]
Patch 4.2: implement test changes to work in e10s mode as well

Skips chat tests for Hello as system addon with e10s enabled

Comment 76

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8705191 [details] [diff] [review]
Patch 2.2: allow to swap docShells on remote browsers that are not a child of a BrowserDOMWindow

No idea how to assess the risk here. Let's uplift it to aurora and see how it goes.

Comment 77

[Carsten Book [:Tomcat]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ecd280b57abb
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/20e2ac26d449
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/0cff5f80c263

Comment 78

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #77)
> remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ecd280b57abb
> remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/20e2ac26d449
> remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/0cff5f80c263

and backed out on request from mike

Comment 79

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8f0068a2287c
https://hg.mozilla.org/releases/mozilla-aurora/rev/73ef63059a6a
https://hg.mozilla.org/releases/mozilla-aurora/rev/3dc44f44572a
https://hg.mozilla.org/releases/mozilla-aurora/rev/96083a1329f5
https://hg.mozilla.org/releases/mozilla-aurora/rev/aac0d928ca69

https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=aac0d928ca69

Comment 80

[Carsten Book [:Tomcat]]

mike, this failed to apply to beta with in part 4:

warning: conflicts while merging browser/extensions/loop/chrome/content/modules/MozLoopService.jsm! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/extensions/loop/chrome/content/preferences/prefs.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Comment 81

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e753a943bd68
https://hg.mozilla.org/releases/mozilla-beta/rev/533cba8abe30
https://hg.mozilla.org/releases/mozilla-beta/rev/cefd1c9093ab
https://hg.mozilla.org/releases/mozilla-beta/rev/06abb1db0df3
https://hg.mozilla.org/releases/mozilla-beta/rev/caa0d1a71205

Comment 82

[Carsten Book [:Tomcat]]

backed this out in https://hg.mozilla.org/releases/mozilla-beta/rev/b454ec296bf6 on request from sylvestre for bustage like :

https://treeherder.mozilla.org/logviewer.html#?job_id=810287&repo=mozilla-beta

Comment 83

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/mozilla-beta/rev/73fb5191562e
https://hg.mozilla.org/releases/mozilla-beta/rev/0ed73f88f2bf
https://hg.mozilla.org/releases/mozilla-beta/rev/da28466abff1
https://hg.mozilla.org/releases/mozilla-beta/rev/d666adbd6569
https://hg.mozilla.org/releases/mozilla-beta/rev/e5952737bbba

Comment 84

[Bogdan Maris, Desktop Test Engineering]

E created a new profile, created prefs.js file with 'browser.tabs.remote.autostart';'true', started Firefox 45 beta 6 and I clicked on Hello icon (basically I forced to have e10s enabled by default in fresh new profile). I could create rooms with ease. Note that 'loop.remote.enabled' is not available in beta though.

Also on Nightly if I shift 'browser.tabs.remote.autostart' to 'false' and back to 'true', I will be able to create rooms as well.

Am I testing the right thing here? Any ideas?

Comment 85

[Carsten Book [:Tomcat]]

backed out by request from sylvestre in https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=8bf2c5452d44

Comment 86

[Mark Banner (:standard8)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #84)
> E created a new profile, created prefs.js file with
> 'browser.tabs.remote.autostart';'true', started Firefox 45 beta 6 and I
> clicked on Hello icon (basically I forced to have e10s enabled by default in
> fresh new profile). I could create rooms with ease. Note that
> 'loop.remote.enabled' is not available in beta though.

I don't think this would have worked in beta 6 - I think we backed out everything before that.

> Also on Nightly if I shift 'browser.tabs.remote.autostart' to 'false' and
> back to 'true', I will be able to create rooms as well.

I think its safe to say some of this is still in progress - see bug 1154273 for outstanding issues.

The main thing to test here for now would be that we still work fine without e10s enabled. Once we get towards enabling e10s, we can do a full shakedown of the e10s code.

Comment 87

[Bogdan Maris, Desktop Test Engineering]

(In reply to Mark Banner (:standard8) from comment #86)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #84)
> > E created a new profile, created prefs.js file with
> > 'browser.tabs.remote.autostart';'true', started Firefox 45 beta 6 and I
> > clicked on Hello icon (basically I forced to have e10s enabled by default in
> > fresh new profile). I could create rooms with ease. Note that
> > 'loop.remote.enabled' is not available in beta though.
> 
> I don't think this would have worked in beta 6 - I think we backed out
> everything before that.
> 
> > Also on Nightly if I shift 'browser.tabs.remote.autostart' to 'false' and
> > back to 'true', I will be able to create rooms as well.
> 
> I think its safe to say some of this is still in progress - see bug 1154273
> for outstanding issues.
> 
> The main thing to test here for now would be that we still work fine without
> e10s enabled. Once we get towards enabling e10s, we can do a full shakedown
> of the e10s code.

Based on this, I think we can safely say that we did test hello without e10s enabled on multiple occasions and we can say that it was verified on 46.

Comment 88

[Bogdan Maris, Desktop Test Engineering]

We recently done a smoketest on Firefox 47 beta 4 across platforms and can safely confirm that Hello works as expected without e10s enabled. I'll go ahead and mark the status of the bug to Verified Fixed.