914926 - workers load on startup even if social.enabled=false

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

turning off social still allows workers to run on restart.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: fix worker startup

review request for either felipe or markh, both not necessary.

The key part of this fix was passing SocialService.enabled to _updateWorkerState in Social.init.  This fix showed up an error in browser_social_multiworker.js where the workers were (prior to fix) loading when Social.enabled is false.

New xpcshell tests allow us to test real startup for Social.jsm.

I noticed inconsistency in the order of notifications for social:providers-changed and social:provider-set, while that shouldn't matter, I also made this behavior consistent, providers-changed now always fires before provider-set.  That also ensures that providers-changed is always sent if Social.providers is updated (it wasn't before, one place missed it).

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

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: fix worker startup

forgot to hg add a file :(

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

Comment 3

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

Comment on attachment 803873 [details] [diff] [review]
fix worker startup

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

A bit of a nit, but please rename the "xpcshell" dir to "unit" - that's far more common and the only name used under "/browser" for xpcshell tests.  I'm also inclined to say that given head.js is non-trivial and very specific to social, there should be a social directory under it - but it that turns out to be a PITA (I don't see many subdirs of "unit", so it might be tricky) then it's ok to keep the tests in the top-level "unit".

Looks good though, and thanks for working out how to test this!

::: browser/modules/Social.jsm
@@ +179,5 @@
>          // installed/uninstalled do not send the providers param
>          Services.obs.notifyObservers(null, "social:" + topic, origin);
>          return;
>        }
> +      if (topic == "provider-enabled") {

I think this change should be unnecessary - the only thing that actually *needed* to change here seems to be Social._updateWorkerState(true) -> Social._updateWorkerState(Social.enabled)?

OTOH though, I guess the new code is also correct - just minor duplication - so no biggie if you think the new code is an improvement.

::: browser/modules/test/xpcshell/head.js
@@ +13,5 @@
> +    name: "provider 1",
> +    origin: "https://example1.com",
> +    sidebarURL: "https://example1.com/sidebar/",
> +  },
> +  { // provider without workerURL

this comment is misleading - the first provider has no workerURL either.

@@ +23,5 @@
> +
> +const MANIFEST_PREFS = Services.prefs.getBranch("social.manifest.");
> +
> +const gProfD = do_get_profile();
> +

please add a bit of a "banner comment" here introducing the AppInfo code and briefly explaining why we need it.

@@ +127,5 @@
> +  SocialService = Cu.import("resource://gre/modules/SocialService.jsm", {}).SocialService;
> +  do_check_eq(SocialService.enabled, enabledOnStartup, "service is doing its thing");
> +  do_check_true(SocialService.hasEnabledProviders, "Service has enabled providers");
> +  Social = Cu.import("resource:///modules/Social.jsm", {}).Social;
> +  do_check_false(Social.initialized, "Social is initialized");

one of the messages should be inverted seeing the condition is (ie, "Social is not initialized")

::: browser/modules/test/xpcshell/test_Social.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Please change the "S" to lower-case - eg, test_social, test_socialDisabledStartup

::: browser/modules/test/xpcshell/test_SocialDisabledStartup.js
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function run_test() {
> +  // we are testing worker startup specifically
> +  Services.prefs.setBoolPref("social.allowMultipleWorkers", true);

reset this pref?

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: fix worker startup

review feedback all added

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

Comment 5

[Shane Caraveo (:mixedpuppy)]

see if I get a clean try after some leaky pushes were backed out

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

Comment 6

[Shane Caraveo (:mixedpuppy)]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Phil Ringnalda (:philor)]

Whee. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d471ec5662, relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/acce93fb65a1, backed out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/d2280ea7c8cc while chasing bug 916757.

Feel free to reland after a debug Windows try push with like 30 browser-chrome runs on WinXP and Win8, since we're still not especially sure what got rid of it.

Comment 9

[Shane Caraveo (:mixedpuppy)]

new try for winxp/8
https://tbpl.mozilla.org/?tree=Try&rev=db6813f9a955

Comment 10

[Shane Caraveo (:mixedpuppy)]

winxp/8 try with bug 904104, bug  914926, bug 914927 and bug 914435

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

Comment 11

[Shane Caraveo (:mixedpuppy)]

a few more runs to finish, but looking green

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

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 804719 [details] [diff] [review]
fix worker startup

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

This was backed out along with several others due to winXP crashes, I narrowed the crash down to bug 914435

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 916757
User impact if declined: service workers run on startup even if social is disabled
Testing completed (on m-c, etc.): fx-team
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ba33700519a9

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Needed build peer review...

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 804719 [details] [diff] [review]
fix worker startup

@khuey, can you review the build changes here?

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 804719 [details] [diff] [review]
fix worker startup

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

::: browser/modules/test/unit/social/Makefile.in
@@ +7,5 @@
> +  head.js \
> +  blocklist.xml \
> +  test_social.js \
> +  test_socialDisabledStartup.js \
> +  $(NULL)

Does this actually do anything?  metro has a libs target to copy the files http://mxr.mozilla.org/mozilla-beta/source/browser/metro/base/tests/Makefile.in#19

Anyways I'm not familiar with the ways we just changed xpcshell stuff so I'm going to punt.

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 804719 [details] [diff] [review]
fix worker startup

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

::: browser/modules/test/unit/social/Makefile.in
@@ +7,5 @@
> +  head.js \
> +  blocklist.xml \
> +  test_social.js \
> +  test_socialDisabledStartup.js \
> +  $(NULL)

This doesn't actually do anything!

You should use "support-files" in the xpcshell.ini manifest for installing extra files.

https://ci.mozilla.org/job/mozilla-central-docs/Build_Documentation/test_manifests.html

Also, you don't need to create a Makefile.in if it contains no content. And, we prefer moz.build files be collapsed into their parent directories if all they define is test manifests. This will make builds faster. In my ideal world, you have structure like:

feature
  moz.build
  tests/
    xpcshell/
      xpcshel.ini
    mochitest/
      chrome.ini
    ...

Please also use "xpcshell" instead of "unit" for the directory name because "unit" is ambiguous.

Comment 19

[Shane Caraveo (:mixedpuppy)]

most of the last comments here were changed at some point in the past.