Closed Bug 914926 Opened 6 years ago Closed 6 years ago

workers load on startup even if social.enabled=false

Categories

(Firefox Graveyard :: SocialAPI, defect)

26 Branch
x86
macOS
defect
Not set

Tracking

(firefox26 fixed, firefox27 fixed)

RESOLVED FIXED
Firefox 27
Tracking Status
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

turning off social still allows workers to run on restart.
Attached patch fix worker startup (obsolete) — Splinter Review
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
Assignee: nobody → mixedpuppy
Attachment #803868 - Flags: review?(mhammond)
Attachment #803868 - Flags: review?(felipc)
Attached patch fix worker startup (obsolete) — Splinter Review
forgot to hg add a file :(

https://tbpl.mozilla.org/?tree=Try&rev=e2c62f03c157
Attachment #803868 - Attachment is obsolete: true
Attachment #803868 - Flags: review?(mhammond)
Attachment #803868 - Flags: review?(felipc)
Attachment #803873 - Flags: review?(mhammond)
Attachment #803873 - Flags: review?(felipc)
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?
Attachment #803873 - Flags: review?(mhammond)
Attachment #803873 - Flags: review?(felipc)
Attachment #803873 - Flags: review+
review feedback all added

https://tbpl.mozilla.org/?tree=Try&rev=ab0a61931bfe
Attachment #803873 - Attachment is obsolete: true
Attachment #804719 - Flags: review+
see if I get a clean try after some leaky pushes were backed out

https://tbpl.mozilla.org/?tree=Try&rev=9e6aeee0159d
https://hg.mozilla.org/mozilla-central/rev/bc8d1b9545c5
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 26 → ---
a few more runs to finish, but looking green

https://tbpl.mozilla.org/?tree=Try&rev=d729b5bd402e
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
Attachment #804719 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/e764f8ec5205
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Attachment #804719 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needed build peer review...
Flags: needinfo?(mixedpuppy)
Comment on attachment 804719 [details] [diff] [review]
fix worker startup

@khuey, can you review the build changes here?
Attachment #804719 - Flags: review?(khuey)
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.
Attachment #804719 - Flags: review?(khuey) → review?(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.
Attachment #804719 - Flags: review?(gps) → review-
Whiteboard: [qa-]
most of the last comments here were changed at some point in the past.
Flags: needinfo?(mixedpuppy)
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.