Mark Hammond [:markh] [:mhammond] Reporter
	Description • 11 years ago

Social activation doesn't work with e10s.  The main problem is that the |document| performing the activation is passed to the installation functions.  While this almost works (a CPOW is passed instead of the real document), _showInstallNotification() fails as _getChromeWindow() fails when passed that window.

The easiest solution may be to pass the <browser> element around instead of the the document, but I haven't actually tried that.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 1 • 11 years ago

I must be missing some context here, since activation works fine for me.  I thought e10s was only being used by the worker, which doesn't come into play with activation.

	Mark Hammond [:markh] [:mhammond] Reporter
	Comment 2 • 11 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> I must be missing some context here, since activation works fine for me.  I
> thought e10s was only being used by the worker, which doesn't come into play
> with activation.

This is about "global" e10s - ie, when the content windows are in their own processes.  This can be dog-fooded now by setting browser.tabs.remote=true.

	Allison Naaktgeboren :ally
	Comment 3 • 10 years ago

This is looking more complicated than that. The "ActivateSocialFeature" events in content aren't reaching the listeners in the parent. (Can't reach the browser.js listener in the content process either. Content process not allowed to touch code below browser/ so can't touch browser-social.js/browser.js where the magic starts)

Let's start there and see how far down the rabbit hole goes.

	:Felipe Gomes (needinfo for replies!)
	Comment 4 • 10 years ago

Stealing this bug. Ally, if you were already working you can take it back, just let me know.

	Marco Mucci [:MarcoM]
	Comment 5 • 10 years ago

Hi Felipe, can you mark this bug as qe-verify+ or qe-verify- for verification.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 6 • 10 years ago

Is there a specific aspect of this we can test?  Markh, from comment 2 it sounds like maybe it is. Thanks!

	u279076
	Comment 7 • 10 years ago

I could schedule a round of smoketesting with the demo provider with e10s enabled. Let me know if that'd be enough to call this verified.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 8 • 10 years ago

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #7)
> I could schedule a round of smoketesting with the demo provider with e10s
> enabled. Let me know if that'd be enough to call this verified.

There is a lot that definitely does not work with e10s right now.  I'm also fairly certain the existing automated tests will fail (e.g. you cannot activate providers, those tests should be failing).  I'm not certain how useful smoke testing would be at this stage.

	Mark Hammond [:markh] [:mhammond] Reporter
	Comment 9 • 10 years ago

(In reply to Liz Henry :lizzard from comment #6)
> Is there a specific aspect of this we can test?  Markh, from comment 2 it
> sounds like maybe it is. Thanks!

I don't think there is anything useful to test yet.  Once Felipe has a patch up, all the tests will presumably work, so then it should be possible to test activation using both the demo provider and the "real" providers we are now hosting on mozilla.net's CDN.  Note that such testing would need to be done in an e10s window.

	:Felipe Gomes (needinfo for replies!)
	Comment 10 • 10 years ago

I'm not actively working on this bug at the moment, so leaving it at NEW if someone wants to pick it up. I believe we still aren't sure if these e10s bugs are going to be tracked through the backlog

	Chris Peterson [:cpeterson]
	Comment 11 • 10 years ago

Move old M2 P3 bugs to M4 (because tracking-e10s=m5+ flag doesn't exist yet).

	u279076
	Comment 12 • 10 years ago

Florin, can you please assign testing this to someone on your team?

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 13 • 10 years ago

This is a bitrotted rough draft to make activation work with e10s, should help as a guide to the basic changes necessary to get activation working.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 14 • 10 years ago

this makes a bit more progress 

@markh: note the comments in the skip-if sections in browser.ini, and the commented out code in browser-child.js, still need to figure that out.  also unsure about whether having both activation handlers is necessary (see SocialUI.init).  input on direction here appreciated.

	Mark Hammond [:markh] [:mhammond] Reporter
	Comment 15 • 10 years ago

Comment on attachment 8494186 [details] [diff] [review]
WIP patch for e10s activation

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

Only a fairly quick look, but I think it is looking good.

::: browser/base/content/browser-social.js
@@ +72,5 @@
>      Services.obs.addObserver(this, "social:provider-disabled", false);
>  
>      Services.prefs.addObserver("social.toast-notifications.enabled", this, false);
>  
> +    // e10s "ActivateSocialFeature" 

trailing spaces :)  But also, I don't think we should have 2 code paths for the remote and non-remote case - just make things unconditionally work with the message manager.  That should still work as expected for content in the parent.

Also, I'd be inclined to use a message name "Social:ActivateFeature" - things seem to be converging on a "{major}:{minor}" naming scheme.

@@ +173,5 @@
> +        dump("***** got activation message "+JSON.stringify(data)+"\n");
> +        dump("---- url is "+data.url+"\n");
> +        dump("---- principal is "+data.origin+"\n");
> +        data.window = window;
> +        SocialUI._installAndActivateProvider(data);

can we use |this| here?

@@ +237,5 @@
>      SocialStatus.populateToolbarPalette();
>      SocialMarks.populateToolbarPalette();
>    },
>  
> +  // This handles non-e10s "ActivateSocialFeature" events fired against content

If we use the message for all activations we can kill this.

::: toolkit/components/social/SocialService.jsm
@@ +507,3 @@
>      if (type == 'directory' || type == 'internal') {
>        // directory provided manifests must have origin in manifest, use that
>        if (!data['origin']) {

it seems a little strange to ignore the passed in origin in this case - can we either assert they match, or the passed origin is null (or whatever it is we expect in that case)?  Then we can just set installOrigin to be data.origin and avoid the |else| and new URI var.

::: toolkit/content/browser-child.js
@@ +243,5 @@
> +addEventListener("ActivateSocialFeature", function (aEvent) {
> +  let win = aEvent.target.defaultView;
> +  let document = content.document;
> +  // We only want to activate if it is as a result of user input.
> +  // XXX I think we want to do this stuff, here, but win is undefined

hmm - content.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils); would work, but I'm not sure what that means in terms of iframes etc.  But best I can tell, event.target.defaultView *should* work here (which in the cases we care about, will actually be |content|)

@@ +252,5 @@
> +  //  return;
> +  //}
> +  //// If we are in PB mode, we silently do nothing (bug 829404 exists to
> +  //// do something sensible here...)
> +  //if (PrivateBrowsingUtils.isWindowPrivate(win))

I'm pretty sure that using |content| here instead of |win| would be OK (but OTOH, once you can get |win| sanely it wouldn't be necessary to change it)

@@ +255,5 @@
> +  //// do something sensible here...)
> +  //if (PrivateBrowsingUtils.isWindowPrivate(win))
> +  //  return;
> +
> +  switch (aEvent.type) {

this switch seems unnecessary seeing we are just handling exactly 1 event, right?

	Dave Townsend [:mossop]
	Comment 16 • 10 years ago

Looks like you're getting on with this Shane

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 18 • 10 years ago

(In reply to Mark Hammond [:markh] from comment #15)
> Comment on attachment 8494186 [details] [diff] [review]
> WIP patch for e10s activation
> 
> Review of attachment 8494186 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Only a fairly quick look, but I think it is looking good.
> 
> ::: browser/base/content/browser-social.js
> @@ +72,5 @@
> >      Services.obs.addObserver(this, "social:provider-disabled", false);
> >  
> >      Services.prefs.addObserver("social.toast-notifications.enabled", this, false);
> >  
> > +    // e10s "ActivateSocialFeature" 
> 
> trailing spaces :)  But also, I don't think we should have 2 code paths for
> the remote and non-remote case - just make things unconditionally work with
> the message manager.  That should still work as expected for content in the
> parent.

We'll have to support 2 code paths until we can make the social iframes remote, otherwise activation from share panel will break.

> > +  // This handles non-e10s "ActivateSocialFeature" events fired against content
> 
> If we use the message for all activations we can kill this.

dito from above

> ::: toolkit/components/social/SocialService.jsm
> @@ +507,3 @@
> >      if (type == 'directory' || type == 'internal') {
> >        // directory provided manifests must have origin in manifest, use that
> >        if (!data['origin']) {
> 
> it seems a little strange to ignore the passed in origin in this case - can
> we either assert they match, or the passed origin is null (or whatever it is
> we expect in that case)?  Then we can just set installOrigin to be
> data.origin and avoid the |else| and new URI var.

data[origin] is meant to be the service origin in this case, so it wont be null, it wont match the install origin.  otherwise simplified this code.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 19 • 10 years ago

> @@ +255,5 @@
> > +  //// do something sensible here...)
> > +  //if (PrivateBrowsingUtils.isWindowPrivate(win))
> > +  //  return;
> > +
> > +  switch (aEvent.type) {
> 
> this switch seems unnecessary seeing we are just handling exactly 1 event,
> right?

Only one in this patch, but bug 1053973 will add a second, and there may be more after that as more stuff gets fixed.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 20 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=80402fb7d6da

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 21 • 10 years ago

fixed a couple tests

https://tbpl.mozilla.org/?tree=Try&rev=a55bc28d69b1

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 22 • 10 years ago

load content.js for the share panel to allow activation to work through the same code path

	Mark Hammond [:markh] [:mhammond] Reporter
	Comment 23 • 10 years ago

Comment on attachment 8517107 [details] [diff] [review]
fix activation under e10s

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

Quite a few trivial nits, but LGTM, thanks!

::: browser/base/content/browser-social.js
@@ +515,5 @@
>      iframe.setAttribute("tooltip", "aHTMLTooltip");
>      iframe.setAttribute("disableglobalhistory", "true");
>      iframe.setAttribute("flex", "1");
>      panel.appendChild(iframe);
> +    iframe.addEventListener("load", function _firstload() {

it would probably be better if DOMContentLoaded worked here, to ensure it is loaded before any scripts in the content execute.

::: browser/base/content/content.js
@@ +839,5 @@
>  
> +addEventListener("ActivateSocialFeature", function (aEvent) {
> +  let document = content.document;
> +  if (PrivateBrowsingUtils.isContentWindowPrivate(content)) {
> +    Cu.reportError("cannot to use providers in private windows");

s/to//, and maybe add the word "social" - "cannot use social providers ..."

@@ +843,5 @@
> +    Cu.reportError("cannot to use providers in private windows");
> +    return;
> +  }
> +  let dwu = content.QueryInterface(Ci.nsIInterfaceRequestor)
> +                  .getInterface(Ci.nsIDOMWindowUtils);

align the '.' chars on these lines.

@@ +850,5 @@
> +    return;
> +  }
> +
> +  let node = aEvent.target;
> +  let ownerDocument = aEvent.target.ownerDocument;

might as well use node.ownerDocument here?

@@ +856,5 @@
> +  if (data) {
> +    try {
> +      data = JSON.parse(data);
> +    } catch(e) {
> +      Cu.reportError("Social Service manifest parse error: "+e);

spaces around operators

@@ +863,5 @@
> +  } else {
> +    Cu.reportError("Social Service manifest not available");
> +    return;
> +  }
> +  

trailing whitespace

::: browser/base/content/test/social/browser_social_activation.js
@@ +89,1 @@
>      waitForCondition(function() {

it looks like you should dedent this block

@@ +167,5 @@
>      }
>    });
> +
> +  activateProvider(manifest.origin, function() {
> +    info("waiting on activation panel to open/close...");

this confused me a little - I think a comment saying "the test will continue as the popup events fire..." would help the next person reading this.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 24 • 10 years ago

fix nits, minor refactor/simplification of activation listener

https://tbpl.mozilla.org/?tree=Try&rev=a003f66a927e

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 25 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/dac7ee29d0f8

	Carsten Book [:Tomcat]
	Comment 26 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/dac7ee29d0f8

	Bogdan Maris, Desktop QA
	Comment 27 • 10 years ago

Reproduced the initial issue using an old Nightly build (2014-10-12). Verified on Windows 7 64bit, Mac OS X 10.9.5 and Ubuntu 14.04 64bit that using latest Nightly with e10s activated, Social Demo and other social providers (Cliqz, delicious, Goal, Mixi etc.) can be activated in latest Nightly.