When sessionrestore takes over to prevent a sequence of crashes, social features should also be prevented to load, because it could be the content of the panels, the worker or the sidebar that causes the crash, leaving the user with an unrecoverable browser.
6 years ago
Assignee: nobody → jaws
Status: NEW → ASSIGNED
6 years ago
We should probably also disable social functionality in safe mode.
6 years ago
Summary: When sessionrestore takes over to prevent a crash, social features should also be disabled → Disable Social API when in session restore or safe mode
Created attachment 656642 [details] [diff] [review] Patch This patch works with disabling the Social API when in safe mode. I need some more time to investigate restoring it upon Session Restore, since Session Restore is optional and the user can continue with a new session without performing any explicit action. I think we can land this now though and move the Session Restore handling to a separate bug.
If I'm not mistaken, that approach will have a bit of a weird effect - social UI won't show up in safe mode, but assuming social has been previously activated the "Motown for Nightly" menu item might still be checked, and toggling it will cause the UI to re-appear, right? I think we need to "disable" it at a lower level (like socialservice.enabled, maybe?)
6 years ago
Created attachment 658095 [details] [diff] [review] Patch v2 (allows for re-enabling) This patch ended up being more complex than I thought it would need to be. I will also attach a simpler patch that doesn't allow for re-enabling. There are a few places that check the enabled pref and in order to allow the feature to be re-enabled while in safe mode I had to introduce an extra state variable to Social.jsm. I also removed the check for setting the pref to the same value. When safe mode is entered, the feature is disabled even if the pref is set to true. Re-enabling the pref would keep it as true, so we can't early-return in this case.
Created attachment 658097 [details] [diff] [review] Patch v2 (does not allow re-enabling) This patch is simpler and is my preferred patch when compared to the previous patch. When in safe mode, the Social features are disabled and do not have a visible way to enable them. Users will have to exit safe mode to enable the features. The Social features could still be enabled/disabled via about:config, but enabling Social this way may produce unexpected behavior while in safe mode.
Would just making SocialService.enabled always return false in safe mode work?
The menu items check for Social.active, so only dealing with SocialService.enabled isn't enough.
The menu item is kind of a special case - it's used for toggling, which we want to disable in safe mode. So we can just have it check Social.active && !safeMode, right?
It'd be nice to have a way in safemode to disable the feature for the next non-safemode run. Is that gonna be possible with this patch? From what I can tell it will
Created attachment 663594 [details] [diff] [review] Patch v2.1 (always off initially, but allows for reenabling) I talked with Gavin, Felipe, and MattN about this patch over the past 2 weeks. Getting this to work correctly is a little complicated, but this patch is also quite small. The way that this works is: - If the feature is enabled prior to safe mode --- The menuitem will show it as being enabled --- All other Social API features will be disabled --- The user can disable Social API by unchecking the menuitem --- The user can re-enable Social API (and its full UI) by re-checking the menuitem - If the feature is disabled prior to safe mode --- The menuitem will be unchecked when safe mode is started --- The full Social API can be enabled by checking the menuitem This will allow a user who is experiencing issues to restart in safe mode, and on-demand disable & enable the Social API as they are trying to narrow down the cause of their issues.
Attachment #663594 - Flags: review?(felipc) → review+
Summary: Disable Social API when in session restore or safe mode → Disable Social API when in safe mode
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d12c34bb82fa for causing: TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/xpcshell/tests/toolkit/components/social/test/xpcshell/test_SocialService.js | test failed (with xpcshell return code: 0), see following log: TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/xpcshell/tests/toolkit/components/social/test/xpcshell/test_SocialService.js | null != null - See following stack:
Landed again with a fix for the test (ok'd in person by Felipe). https://hg.mozilla.org/integration/mozilla-inbound/rev/6c68ef18d340
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Attachment #663594 - Flags: approval-mozilla-aurora+
status-firefox17: --- → fixed
Verified on Firefox 17 beta 5 that Social API is disabled in safe mode and works as mentioned in Comment 10. Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.8: Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Firefox/17.0 Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:17.0) Gecko/17.0 Firefox/17.0
status-firefox17: fixed → verified
mass remove verifyme requests greater than 4 months old
You need to log in before you can comment on or make changes to this bug.