Closed
Bug 786095
Opened 12 years ago
Closed 12 years ago
Disable Social API when in safe mode
Categories
(Firefox Graveyard :: SocialAPI, defect)
Firefox Graveyard
SocialAPI
Tracking
(firefox17 verified)
RESOLVED
FIXED
Firefox 18
Tracking | Status | |
---|---|---|
firefox17 | --- | verified |
People
(Reporter: Felipe, Assigned: jaws)
Details
(Whiteboard: [Fx17])
Attachments
(1 file, 3 obsolete files)
4.52 KB,
patch
|
Felipe
:
review+
Gavin
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Assignee | ||
Updated•12 years ago
|
Whiteboard: [Fx17]
Comment 1•12 years ago
|
||
We should probably also disable social functionality in safe mode.
Assignee | ||
Updated•12 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
Assignee | ||
Comment 2•12 years ago
|
||
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.
Attachment #656642 -
Flags: review?(felipc)
Comment 3•12 years ago
|
||
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?)
Assignee | ||
Updated•12 years ago
|
Attachment #656642 -
Flags: review?(felipc)
Assignee | ||
Comment 4•12 years ago
|
||
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.
Attachment #656642 -
Attachment is obsolete: true
Attachment #658095 -
Flags: review?(felipc)
Assignee | ||
Comment 5•12 years ago
|
||
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.
Attachment #658097 -
Flags: review?(felipc)
Comment 6•12 years ago
|
||
Would just making SocialService.enabled always return false in safe mode work?
Assignee | ||
Comment 7•12 years ago
|
||
The menu items check for Social.active, so only dealing with SocialService.enabled isn't enough.
Comment 8•12 years ago
|
||
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?
Reporter | ||
Comment 9•12 years ago
|
||
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
Assignee | ||
Comment 10•12 years ago
|
||
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 #658095 -
Attachment is obsolete: true
Attachment #658097 -
Attachment is obsolete: true
Attachment #658095 -
Flags: review?(felipc)
Attachment #658097 -
Flags: review?(felipc)
Attachment #663594 -
Flags: review?(felipc)
Reporter | ||
Updated•12 years ago
|
Attachment #663594 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 11•12 years ago
|
||
Flags: in-testsuite-
Summary: Disable Social API when in session restore or safe mode → Disable Social API when in safe mode
Assignee | ||
Comment 12•12 years ago
|
||
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:
Assignee | ||
Comment 13•12 years ago
|
||
Landed again with a fix for the test (ok'd in person by Felipe).
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c68ef18d340
Comment 14•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Updated•12 years ago
|
Attachment #663594 -
Flags: approval-mozilla-aurora+
Comment 15•12 years ago
|
||
status-firefox17:
--- → fixed
Comment 16•12 years ago
|
||
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
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•