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).

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

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

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.

> 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.

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

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

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

(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.

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

correct patch

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 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 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)

This version simply removes the pref

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 :)

(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.)

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

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.

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.

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

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 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.

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

(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 on attachment 822022 [details] [diff] [review] Test workaround Tentatively requesting review.

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.

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+ :)

browser_frameworker_sandbox.js also timeouts on ASAN builds.

Comment on attachment 822226 [details] [diff] [review] Longer timeout also for browser_frameworker_sandbox.js https://tbpl.mozilla.org/?tree=Try&rev=eba6191f3cdd

I suppose disabling the test for now is fine. Can we just split the tests, rather than bumping the timeout?

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 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).

Per Gavin's request in comment 32.

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).

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

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)

(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.

(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?

(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?

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)

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

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

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.

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

(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?

(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?

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.

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

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,

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

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 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.

(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.

(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

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.

(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 :-)).

(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.

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?

(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.