906839 - enable by default social.allowMultipleWorkers

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

bug 891216 introduces the ability to run the workers for all installed providers (that use them).  We need remote frameworkers and better delayed startup of workers before we pref this on (by removing the pref code).

Comment 1

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

I expect we will also want bug 911639 to land before enabling this.

Comment 2

[Nicholas Nethercote [inactive]]

What's the desired timeline for this bug?  A rough estimate is ok... tomorrow?  Next week?  Next month?  Thanks.

Comment 3

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

Ideally (IMO) it could land now and sit on Aurora for 2 cycles (ie, not ride the train from Aurora to Beta, just like the background thumbnails are likely to do).  However, given the other blockers, it's probably not going to happen before the next merge.

Comment 5

[Nicholas Nethercote [inactive]]

> I expect we will also want bug 911639 to land before enabling this.

Turns out bug 911639 was invalid, and so doesn't block this.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: enable multiple workers by default

This turned up some issues in the tests I had to address, as well as one bug in updating the provider enabled state, but it all looks to be working now.

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

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: enable multiple workers by default

fix toolkit test

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

Comment 8

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

Comment on attachment 815869 [details] [diff] [review]
enable multiple workers by default

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

This seems to be going down the right path, but r- for:

* Confusion (probably mine) about the "aurora/beta cycles" comment in firefox.js - see my comment there.

* Some seemingly unrelated test changes - while I understand that flipping this pref might cause some test tweaks as being necessary, I think they should be able to be rolled up into their own patch that is independent of the pref being flipped - this will make it easier to rationalize the test changes.

::: browser/app/profile/firefox.js
@@ +1283,5 @@
>  
>  pref("social.sidebar.open", true);
>  pref("social.sidebar.unload_timeout_ms", 10000);
>  
> +// bug 906839 pref on during aurora/beta cycles

I don't understand this comment about aurora/beta cycles - it looks like it is unconditional?  If the intent is that it be unconditional, I think it would be better to keep the pref out of here (ie, I doubt it is something we want to keep as a pref over the long term)

::: browser/base/content/test/social/browser_social_chatwindow.js
@@ +63,5 @@
>      ok(chats.children.length == 0, "no chatty children left behind");
>      cb();
>    };
> +  // always run chat tests with multiple workers.
> +  Services.prefs.setBoolPref("social.allowMultipleWorkers", true);

is this necessary now the default for the pref is true?

@@ +73,2 @@
>      runSocialTests(tests, undefined, postSubTest, function() {
> +      Services.prefs.clearUserPref("social.allowMultipleWorkers");

ditto here - doesn't it remain true after this?

@@ +576,5 @@
>      }
> +    // make sure a user profile is set for this provider as chat windows are
> +    // only closed on *change* of the profile data rather than merely setting
> +    // profile data.
> +    port.postMessage({topic: "test-set-profile"});

the way this comment is written, it sounds like a different bug - ISTM that "merely setting profile data" should close chat windows that were opened before the profile data was set.

::: toolkit/components/social/FrameWorker.jsm
@@ +198,5 @@
>      // idea is that there is no point in having people help test multiple
>      // "old" frameworkers - so anyone who wants multiple workers is forced to
>      // help us test remote frameworkers too.
> +    try {
> +      if (Services.prefs.getBoolPref("social.allowMultipleWorkers")) {

please move the setAttribute out of the exception handler just incase it throws.  Eg:

let useMultiProvider
try {
  useMultiProvider =   ...getBoolPref...
} catch (ex) {
  useMultiProvider = false;  // ...
}
if (useMultiProvider)
  ... setAttribute

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond [:markh] from comment #8)
> Comment on attachment 815869 [details] [diff] [review]
> enable multiple workers by default
> 
> Review of attachment 815869 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems to be going down the right path, but r- for:
> 
> * Confusion (probably mine) about the "aurora/beta cycles" comment in
> firefox.js - see my comment there.

hopefully the new comment is clearer.

> * Some seemingly unrelated test changes - while I understand that flipping
> this pref might cause some test tweaks as being necessary, I think they
> should be able to be rolled up into their own patch that is independent of
> the pref being flipped - this will make it easier to rationalize the test
> changes.

I split the pref change itself out to a separate patch.

> ::: browser/base/content/test/social/browser_social_chatwindow.js
> @@ +63,5 @@
> >      ok(chats.children.length == 0, "no chatty children left behind");
> >      cb();
> >    };
> > +  // always run chat tests with multiple workers.
> > +  Services.prefs.setBoolPref("social.allowMultipleWorkers", true);
> 
> is this necessary now the default for the pref is true?

I want the tests to work regardless of the pref setting.  If we pref off later, we only need to back out the pref setting itself.

> @@ +576,5 @@
> >      }
> > +    // make sure a user profile is set for this provider as chat windows are
> > +    // only closed on *change* of the profile data rather than merely setting
> > +    // profile data.
> > +    port.postMessage({topic: "test-set-profile"});
> 
> the way this comment is written, it sounds like a different bug - ISTM that
> "merely setting profile data" should close chat windows that were opened
> before the profile data was set.

The closing of chat windows based on profile changes works exactly as intended (bug 840870), however this test file relied on behavior caused by toggling Social.enabled.  We no longer do that so we need to be explicit about the profile actually changing.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: update tests to work pref'd on, also fixes a missing update to the worker state

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: fix a port closed error in earlier try tests

a couple other tests failed after an error message that pointed me here.  not entirely sure they are related (they dont seem so), but this cleans up the first error.  See the linux bc failures in https://tbpl.mozilla.org/?tree=Try&rev=d3b6c7d18054

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: fix a port closed error in earlier try tests

correct patch

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: flip the allowMultipleProvider pref

Depends on the updated tests for passing.

try with fixed tests, pref'd off by default:
https://tbpl.mozilla.org/?tree=Try&rev=bf344b7801e0

note: both test fixes are actually there though the big one is not showing in try, it was pushed to try in https://tbpl.mozilla.org/?tree=Try&rev=1634b3407be2

try with fixed tests, pref'd on by default:

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

Comment 14

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

Comment on attachment 816320 [details] [diff] [review]
update tests to work pref'd on, also fixes a missing update to the worker state

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

::: browser/base/content/test/social/browser_social_chatwindow.js
@@ +573,5 @@
>            }
>            break;
>        }
>      }
> +    // make sure a user profile is set for this provider as chat windows are

I think this comment needs to change as it still confused me even after reading your explanation.  Something like:  "we need to have a logged in user so when we log out it is seen as a profile state change and we close the chat window".  Assuming that's correct - otherwise I'm still missing something :)

::: browser/modules/Social.jsm
@@ +95,5 @@
>    _disabledForSafeMode: false,
>  
>    get allowMultipleWorkers() {
> +    try {
> +      return Services.prefs.getBoolPref("social.allowMultipleWorkers");

this doesn't seem necessary?

::: toolkit/components/social/test/browser/browser_SocialProvider.js
@@ +60,5 @@
>        name: "Example Provider 2",
>        workerURL: "http://test2.example.com/browser/toolkit/components/social/test/browser/worker_social.js"
>      };
>      SocialService.addProvider(manifest, function (provider2) {
>        ok(provider.enabled, "provider is initially enabled");

please have only the pref fetch in the exception handler - eg:
let shouldBeEnabled;
try {
  shouldBeEnabled = Services.prefs.....;
} catch (_) {
  shouldBeEnabled = false;
}
is(provider2.enabled, shouldBeEnabled, "...");

Comment 15

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

Comment on attachment 816323 [details] [diff] [review]
flip the allowMultipleProvider pref

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

I think we should just nuke the pref, and the code that checks it, and the comments around it - otherwise we just grow cruft that never gets removed.  The patch should be reasonable and easy to back out if needed (I think - I'd like to see what that patch looks like though)

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: remove the allowMultipleWorkers pref

This version simply removes the pref

Comment 17

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

Comment on attachment 817770 [details] [diff] [review]
remove the allowMultipleWorkers pref

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

It looks like this isn't on top of the other reviewed patches here but it should be (eg, all those changes to browser_social_chatwindow look like they shouldn't be in this patch) - r=me once it is rebased to be on top of the others here.

I had a quick chat with Gavin and he is comfortable with enabling this on 27, so I think you can just go for it (yay!)

::: browser/base/content/test/social/browser_social_status.js
@@ +50,2 @@
>        Services.prefs.clearUserPref("social.whitelist");
>        

might as well nuke this trailing whitespace seeing we are so close to it :)

Comment 18

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

(although keep the other dependencies in mind.  Of particular note, all providers and thumbnails will share the same remote process (there is a pref to limit the number of processes with the default being 1) - so a crash in thumbnails or 1 provider would mean all providers fail - and I don't think we really have a way to restart them other than to restart the browser.)

Comment 19

[Shane Caraveo (:mixedpuppy)]

I'm off to vacation, Florian will finish up here.

Comment 20

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

Comment on attachment 816323 [details] [diff] [review]
flip the allowMultipleProvider pref

Talking with Florian, I now agree this is a more prudent patch to land now - we can nuke the pref later.

Comment 21

[Florian Quèze [:florian]]

Rephrasing the summary to match what's actually going to happen here. I'll file another bug to remove the pref; and I expect us to land the pref removal as soon as mozilla-central is moz28.

Comment 22

[Florian Quèze [:florian]]

I pushed the patches to try to check that they haven't bitrotted:
https://tbpl.mozilla.org/?tree=Try&rev=fa2ef23bf9c2

Except for 2 failures that look both intermittent and unrelated, it was green.


https://hg.mozilla.org/integration/fx-team/rev/9eaf00ec189a
https://hg.mozilla.org/integration/fx-team/rev/6c3eb5fd4ade
https://hg.mozilla.org/integration/fx-team/rev/cfd30965638a

Comment 23

[Ed Morley [:emorley]]

Backed out for increasing the failure rate of browser_frameworker.js on debug runs (the try push was opt-only), eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29608639&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=29605506&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=29607939&tree=Fx-Team

remote:   https://hg.mozilla.org/integration/fx-team/rev/0e73515ec46a
remote:   https://hg.mozilla.org/integration/fx-team/rev/9558ebb23f8d
remote:   https://hg.mozilla.org/integration/fx-team/rev/45a18b183676

Comment 24

[Florian Quèze [:florian]]

Comment on attachment 817770 [details] [diff] [review]
remove the allowMultipleWorkers pref

Marking this attachment as obsolete, as I filed bug 930641 to handle removing the pref.

Comment 25

[Florian Quèze [:florian]]

Attachment: Test workaround

Workaround suggested by markh, assuming the failures we saw in Linux Debug and ASAN builds was bug 919878.

Comment 26

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #25)
> Created attachment 822022 [details] [diff] [review]
> Test workaround

I pushed this to try (https://tbpl.mozilla.org/?tree=Try&rev=2e97837be791) and retriggered 10 times the mochitest-browser-chrome test suite for Linux debug, Linux 64 debug and Linux 64 ASAN (ie. the 3 configurations where the test failed on Fx-Team).

Comment 27

[Florian Quèze [:florian]]

Comment on attachment 822022 [details] [diff] [review]
Test workaround

Tentatively requesting review.

Comment 28

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

Comment on attachment 822022 [details] [diff] [review]
Test workaround

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

Failed :( Turns out the issue in bug 919878 is initialization of each browser element, not of the child process itself.

Comment 29

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

Attachment: Disable one frameworker sub-test and requestLongerTimeout

This attachment fixes the test issues found in the previous landing.  It disables one social tests that quickly creates a frameworker then immediately destroys it - which can trigger bug 919878.  Given that deleting the frameworker immediately after creation is really only going to happen in tests such as this, I'm fairly comfortable that it doesn't point to something social users are going to see - and that bug doesn't cause a "real" crash (but does lead to the child process being killed and a crash to be reported via the crash events)

There's an almost-green try run at https://tbpl.mozilla.org/?tree=Try&rev=5745cbbe8ef2 - ASAN builds are still failing there, but the logs tell me this is simply due to ASAN being slow - the test is passing, but just times out just before the end.  This isn't surprising - the test does alot and a remote frameworker will slow it down.  I've added a requestLongerTimeout for this - new try at https://tbpl.mozilla.org/?tree=Try&rev=8f7685df6dd7

While I'm relatively comfortable with this, I'm requesting review from Gavin so he's explicitly aware of what is going on here.  FWIW, the talkilla team are very keen to get this into 27, so Florian or I will land this as soon as/if it gets r+ :)

Comment 30

[Florian Quèze [:florian]]

Attachment: Longer timeout also for browser_frameworker_sandbox.js

browser_frameworker_sandbox.js also timeouts on ASAN builds.

Comment 31

[Florian Quèze [:florian]]

Comment on attachment 822226 [details] [diff] [review]
Longer timeout also for browser_frameworker_sandbox.js

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

Comment 32

[:Gavin Sharp [email: [e-mail]]]

I suppose disabling the test for now is fine.

Can we just split the tests, rather than bumping the timeout?

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 816323 [details] [diff] [review]
flip the allowMultipleProvider pref

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js

>+// bug 906839 pref on by default during the train, remove pref later after we
>+// know we will not pref off again
>+pref("social.allowMultipleWorkers", true);

This comment is confusing, just omit it. Having a bug on file to remove the pref is sufficient.

Comment 34

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 816320 [details] [diff] [review]
update tests to work pref'd on, also fixes a missing update to the worker state

Please don't add the try/catches here (Mark mentioned this, but wanted to make sure it wasn't forgotten).

Comment 35

[Florian Quèze [:florian]]

Attachment: Split long frameworker tests

Per Gavin's request in comment 32.

Comment 36

[Florian Quèze [:florian]]

Attachment: Without try/catch

attachment 816320 [details] [diff] [review] and attachment 816323 [details] [diff] [review] merged into one to remove the try/catches, per IRC discussion with Gavin and his comment 34 (also addresses comment 33).

Comment 37

[Florian Quèze [:florian]]

Try push for the new patches: https://tbpl.mozilla.org/?tree=Try&rev=c9fe08e6851b

Comment 38

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 822255 [details] [diff] [review]
Without try/catch

try/catch removal looks good, I didn't review this carefully (but I don't think I need to, given mark's previous review - if you think otherwise please re-request review from him)

Comment 39

[Florian Quèze [:florian]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #38)

> try/catch removal looks good, I didn't review this carefully (but I don't
> think I need to, given mark's previous review - if you think otherwise
> please re-request review from him)

Thanks. Mark's previous review will be enough, I just wanted to ensure you were happy with the changes compared to the previously r+'ed attachments.

Comment 40

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #31)
> Comment on attachment 822226 [details] [diff] [review]
> Longer timeout also for browser_frameworker_sandbox.js
> 
> https://tbpl.mozilla.org/?tree=Try&rev=eba6191f3cdd

This try run is completely green.

(In reply to Florian Quèze [:florian] [:flo] from comment #37)
> Try push for the new patches:
> https://tbpl.mozilla.org/?tree=Try&rev=c9fe08e6851b

On this run with the tests split instead of using a longer timeout, all the tests are orange on the 'Linux x64 ASAN' build. The 4 parts (browser_frameworker.js, browser_frameworker_2.js, browser_frameworker_sandbox.js and browser_frameworker_sandbox_2.js) all timeout. The last test of browser_frameworker.js that is executed correctly isn't the same on all logs; it seems the child process crashes (or the parent process thinks it crashes) a bit randomly. I have no idea of how splitting the tests could cause this. Any help with this would be very appreciated.

Gavin, would you be open to us landing the longer timeouts (attachment 822226 [details] [diff] [review]) so that this bug can land before the moz27 merge to aurora? Maybe after pushing it to try again to ensure it's consistently green?

Comment 41

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

(In reply to Florian Quèze [:florian] [:flo] from comment #40)
> I have no idea of how splitting the tests could cause this.

It didn't :(  Looking at my try run where only browser_frameworker had the longer timeout - https://tbpl.mozilla.org/?tree=Try&rev=8f7685df6dd7 - the first few failures show browser_frameworker_sandbox failing, but 2 of them show the same symptoms as this - browser_frameworker timing out "for real" for reasons I don't understand - but I'd guess some kind of race condition with process creation (eg, attempting to create the browser in a process that is in the process of terminating)

> Gavin, would you be open to us landing the longer timeouts (attachment
> 822226 [details] [diff] [review]) so that this bug can land before the moz27
> merge to aurora?

As above, that looks like its going to have the exact same problem.

On the assumption I'm correct that the issue is a race at process shutdown time, I pushed a try run at https://tbpl.mozilla.org/?tree=Try&rev=d27ae76ea296, which has the same workaround we previously tried - a trick that should keep the remote process alive for the entire test duration.  That seems green(-ish) - however, it means we are working around a bug that doesn't seem to have been filed, and thus is a total unknown.  This makes me relatively uncomfortable about landing it :(  OTOH, backing out multiple frameworkers is trivial if real problems can be identified, so I'm torn...

Gavin: thoughts?

Comment 42

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

I should have mentioned - the change with the workaround is at https://hg.mozilla.org/try/rev/b878c3b98d61 - see the "keepAlive" variable.

Re the setLongerTimeout() - I personally think they are fine and somewhat better than artificially splitting the tests with the code duplication that entail.  Further, I think the test runner should automatically have a longer timeout for ASAN runs - they are known to be *alot* slower - so I think either of those 2 options are fine in a followup (if at all)

Comment 43

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

Attachment: 0003-Bug-906839-disable-one-social-test-due-to-bug-919878.patch

This patch just disables the one test due to bug 919878 - it no longer splits the tests due to ASan builds being disabled.  Carrying Gavin's r+ forward sounds reasonable here - this patch now does less than the previous one with r+.

Full try at https://tbpl.mozilla.org/?tree=Try&rev=90523b87abc3

Comment 44

[Florian Quèze [:florian]]

I pushed this one last time to try today (https://tbpl.mozilla.org/?tree=Try&rev=b63d41fd8e88) and apart from the 'bc' on Linux ASAN that takes too long (but is currently hidden by default, see bug 932164), it looked green, so I guess it's time for relanding on fx-team:

https://hg.mozilla.org/integration/fx-team/rev/68960c6bdce3
https://hg.mozilla.org/integration/fx-team/rev/3ae60bdf3fef
https://hg.mozilla.org/integration/fx-team/rev/cf64922197ca

Comment 45

[Phil Ringnalda (:philor)]

Um, ASan isn't disabled, or something you can knowingly break just because it's hidden because someone else broke it while it was hidden for something else, probably Social leaks, breaking it. If you want to punt on having your test get the benefits of ASan, skip your test on it.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

What philor said. Backed out.
https://hg.mozilla.org/integration/fx-team/rev/311650f88451

Comment 47

[Florian Quèze [:florian]]

(In reply to Phil Ringnalda (:philor) from comment #45)
> Um, ASan isn't disabled, or something you can knowingly break

Comment 44 was poorly worded, sorry. The test doesn't fail on ASAN, it only intermittently causes "This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort.", slightly more often than before.

Our (markh and I) understanding of the situation for bc tests on ASAN is that they take much longer to run on ASAN and should have a longer timeout by default for all tests of the bc test suite. Isn't this already covered by bug 932164?

Comment 48

[Florian Quèze [:florian]]

(In reply to Phil Ringnalda (:philor) from comment #45)
> If you want to punt on having your
> test get the benefits of ASan, skip your test on it.

Is there a way to do this without skipping the test on all linux builds?

Comment 49

[Phil Ringnalda (:philor)]

Well, the period of the intermittency turned out to be 0%, every single one since you pushed hit that failure :)

All that bug 932164 and bug 932159 were about was the way that the leading edge of what caused the multiday tree closure for OOM from leaks hit ASan first and hardest. It actually got fixed for that by a patch landed from bug 932159 even before the closure, but then during the time that it was hidden, someone started another very frequent failure, bug 934641, which I was hoping we could clear up before unhiding it.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/aboutmemory/tests/Makefile.in#6 seems to say that ifndef MOZ_ASAN lets you punt, though I'd test it and make sure, and make sure you're still running on non-ASan.

Comment 50

[Florian Quèze [:florian]]

Attachment: Disable frameworker tests on ASAN

As suggested in comment 49. I tested this in https://tbpl.mozilla.org/?tree=Try&rev=a7b1d2263bf4 and it worked as expected.

Comment 51

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

Comment on attachment 827631 [details] [diff] [review]
Disable frameworker tests on ASAN

ASan seems to have a mind of its own, and given the rest of the social test suite remains enabled, I'm comfortable with skipping these tests on ASan,

Comment 52

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/cc6a86be98fd
https://hg.mozilla.org/integration/fx-team/rev/ba3ea4d6eaf4
https://hg.mozilla.org/integration/fx-team/rev/d7e1c603d06c
https://hg.mozilla.org/integration/fx-team/rev/c873d63c48d3

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cc6a86be98fd
https://hg.mozilla.org/mozilla-central/rev/ba3ea4d6eaf4
https://hg.mozilla.org/mozilla-central/rev/d7e1c603d06c
https://hg.mozilla.org/mozilla-central/rev/c873d63c48d3

Comment 54

[Florian Quèze [:florian]]

Comment on attachment 822255 [details] [diff] [review]
Without try/catch

[Approval Request Comment]
Note: I'm putting the approval-mozilla-aurora? flag on this patch (the largest of the 4), but I'm actually requesting approval for the 4 changesets that landed in comment 53.

I pushed this to try above the current mozilla-aurora tip, and it looks green: https://tbpl.mozilla.org/?tree=Try&rev=dff8268e719b

Bug caused by (feature/regressing bug #): Multiple social providers.
User impact if declined: Without the changes we made here, only the 'active' social provider can have a running worker. This prevents implementing communication applications as a social provider, as incoming calls wouldn't be received when the social provider is not the active one. We are expecting to bring Talkilla (https://wiki.mozilla.org/Talkilla) to a larger audience within the Firefox 27 timeframe, and this bug is a blocker for this.
Testing completed (on m-c, etc.): It has been baking on m-c for more than a week, and there are tests.
Risk to taking this patch (and alternatives if risky): Moderate, but it's easy to just set the preference to 'false' on branches if we really have to.
String or IDL/UUID changes made by this patch: None.

Comment 55

[bhavana bajaj [:bajaj]]

(In reply to Florian Quèze [:florian] [:flo] from comment #54)
> Comment on attachment 822255 [details] [diff] [review]
> Without try/catch
> 
> [Approval Request Comment]
> Note: I'm putting the approval-mozilla-aurora? flag on this patch (the
> largest of the 4), but I'm actually requesting approval for the 4 changesets
> that landed in comment 53.
> 
> I pushed this to try above the current mozilla-aurora tip, and it looks
> green: https://tbpl.mozilla.org/?tree=Try&rev=dff8268e719b
> 
> Bug caused by (feature/regressing bug #): Multiple social providers.
> User impact if declined: Without the changes we made here, only the 'active'
> social provider can have a running worker. This prevents implementing
> communication applications as a social provider, as incoming calls wouldn't
> be received when the social provider is not the active one. We are expecting
> to bring Talkilla (https://wiki.mozilla.org/Talkilla) to a larger audience
> within the Firefox 27 timeframe, and this bug is a blocker for this.
> Testing completed (on m-c, etc.): It has been baking on m-c for more than a
> week, and there are tests.
> Risk to taking this patch (and alternatives if risky): Moderate, but it's
> easy to just set the preference to 'false' on branches if we really have to.
> String or IDL/UUID changes made by this patch: None.

Has there been any testing around this area of uplift ? If not, please get in touch with :tracy(QA contact) to discuss a testplan so we have our grounds covered.

Comment 56

[Florian Quèze [:florian]]

(In reply to bhavana bajaj [:bajaj] from comment #55)

Thanks for the approval!

> Has there been any testing around this area of uplift ? If not, please get
> in touch with :tracy(QA contact) to discuss a testplan so we have our
> grounds covered.

I started an email thread to discuss this.

https://hg.mozilla.org/releases/mozilla-aurora/rev/e75cd4c5c2a1
https://hg.mozilla.org/releases/mozilla-aurora/rev/668f855471bb
https://hg.mozilla.org/releases/mozilla-aurora/rev/f9f39c252d53
https://hg.mozilla.org/releases/mozilla-aurora/rev/d37821195af4

Comment 57

[Tracy Walker [:tracy]]

A basic place for QA to start here is how to verify this fix? I'm sorry, I'm not actually familiar with Talkilla and the like, so some STR's would be great. Thanks ahead of time.

Comment 58

[Florian Quèze [:florian]]

(In reply to [:tracy] Tracy Walker - QA Mentor from comment #57)
> A basic place for QA to start here is how to verify this fix? I'm sorry, I'm
> not actually familiar with Talkilla and the like, so some STR's would be
> great. Thanks ahead of time.

A way to check that the intended change works is:
1. Install talkilla (https://talkilla.mozillalabs.com/) and login.
2. Install another SocialAPI provider and ensure its sidebar is visible.
3. From another Firefox instance with Talkilla, call the first instance.

Expected result:
You should have a chat window appearing prompting you to accept the call.

Without the patch:
No call window appearing if Talkilla isn't the selected social provider. And actually, at step 2 the first instance would have been signed out, and will be no longer marked as available from the point of view of the second instance.


To be honest, I'm not really concerned about whether the fix works or not (I'm confident it does; and I there are plenty of unit tests). The concerns are more about things the patch could break (hopefully nothing :-)).

Comment 59

[Shane Caraveo (:mixedpuppy)]

(In reply to Florian Quèze [:florian] [:flo] from comment #58)
> (In reply to [:tracy] Tracy Walker - QA Mentor from comment #57)
> > A basic place for QA to start here is how to verify this fix? I'm sorry, I'm
> > not actually familiar with Talkilla and the like, so some STR's would be
> > great. Thanks ahead of time.
> 
> A way to check that the intended change works is:

> 2. Install another SocialAPI provider and ensure its sidebar is visible.

There is general testing and verification which is done using the demo/test provider at http://mixedpuppy.github.io/socialapi-demo/  I'm not sure what process Anthony goes through when he does general verification.

> To be honest, I'm not really concerned about whether the fix works or not
> (I'm confident it does; and I there are plenty of unit tests). The concerns
> are more about things the patch could break (hopefully nothing :-)).

I'll second that, though there are a lot of socialapi tests in general, so I'm not too concerned.  There are always unknown issues that can crop up.

Comment 60

[Tracy Walker [:tracy]]

ok, I confirmed this doesn't work pre-fix and does work following STR's in comment #58.  Going to have to do some exploratory testing to see if other things are broken. 

Are there any areas you particularly concerned this might break?

Comment 61

[Florian Quèze [:florian]]

(In reply to [:tracy] Tracy Walker - QA Mentor from comment #60)

> Are there any areas you particularly concerned this might break?

I think it could be useful to verify that the SocialAPI providers that work in Firefox 25 still work after these changes. I think the only difference the changes here could do is introduce a bit more latency when passing messages between a provider's UI and its worker; so it would be reasonable to assume this won't affect the existing providers... but we could have surprises.