Closed Bug 820613 Opened 7 years ago Closed 5 years ago
Remove all references to "shutdown-cleanse"
From bug 807757 comment 5: > +NOTE: Long ago there was be a "shutdown-cleanse" version of shutdown which was > +intended to clear profile data. This is no longer sent and observer code should > +remove support for it.
Does this also include the SHUTDOWN_CLEANSE and SHUTDOWN_PERSIST variables @ http://dxr.mozilla.org/mozilla-central/--GENERATED--/profile/public/_xpidlgen/nsIProfile.h.html#l60 ?
Updated patch and to tip
Comment on attachment 695409 [details] [diff] [review] Patch that removes references to shutdown-cleanse netwerk/cache/nsCacheService.cpp doesn't look right. You removed a call to nsCacheService::OnProfileShutdown completely, when I believe that you should only have removed the `cleanse` parameter to that method. In nsCertOverrideService.cpp and nsCookieService.cpp you have kept code that should have been removed (I think misread the "!NS_strcmp" call.)
Attachment #695409 - Flags: review?(benjamin) → review-
(In reply to Benjamin Smedberg [:bsmedberg] from comment #4) > netwerk/cache/nsCacheService.cpp doesn't look right. You removed a call to > nsCacheService::OnProfileShutdown completely, when I believe that you should > only have removed the `cleanse` parameter to that method. I shouldn't have removed it. So, after reading profile-before-change on https://developer.mozilla.org/en-US/docs/Observer_Notifications again, I'm not sure what needs to be done for http://dxr.mozilla.org/mozilla-central/netwerk/cache/nsCacheService.cpp.html?string=nsCacheService.cpp#l2363 and #l2371 as 'cleanse' can't be true once shutdown_cleanse's removed > In nsCertOverrideService.cpp and nsCookieService.cpp you have kept code that > should have been removed (I think misread the "!NS_strcmp" call.) So, w.r.t nsCertOverrideService.cpp, I assume 'deletion of storage file' part at http://dxr.mozilla.org/mozilla-central/security/manager/ssl/src/nsCertOverrideService.cpp.html#l151 is not required anymore as it's handled elsewhere w.r.t nsCookieService.cpp, the if loop @ http://dxr.mozilla.org/mozilla-central/netwerk/cookie/nsCookieService.cpp.html#l1445 can be removed as 'shutdown-cleanse' is not part of the condition anymore Can you confirm?
remove the `cleanse` parameter and refactor the code as if it were never true. For certoverride, just remove that entire block (line 149-154)
Assignee: nobody → kshriram18
Backed out for xpcshell failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/d802d6faa080 https://tbpl.mozilla.org/php/getParsedLog.php?id=19166337&tree=Mozilla-Inbound TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/extensions/cookie/test/unit/test_cookies_persistence.js | test failed (with xpcshell return code: 0), see following log: TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/extensions/cookie/test/unit/head_cookies.js | 4 == 1 - See following stack: TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/extensions/cookie/test/unit/head_cookies.js | caught exception 2147500036 - See following stack: TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/extensions/cookie/test/unit/test_cookies_thirdparty_session.js | test failed (with xpcshell return code: 0), see following log: TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/extensions/cookie/test/unit/head_cookies.js | 4 == 1 - See following stack:
Try run for c93a65e68983 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=c93a65e68983 Results (out of 116 total builds): exception: 82 success: 26 warnings: 8 Builds (or logs if builds failed) available at: http://email@example.com
Latest Try Info: Results will be displayed on TBPL as they come in: https://tbpl.mozilla.org/?tree=Try&rev=99b8a9a3b478 Once completed, builds and logs will be available at: http://firstname.lastname@example.org
Duplicate of this bug: 846760
The removal of http://dxr.mozilla.org/mozilla-central/netwerk/cookie/nsCookieService.cpp#l1447 got rid of the RemoveAll() call which clears the cookies via hostTable.clear() in RemoveAllFromMemory Now this's reason the tests @ http://dxr.mozilla.org/mozilla-central/extensions/cookie/test/unit/test_cookies_persistence.js#l64 and thirdparty_session fail as seen in try log above. Now with support for 'shutdown-cleanse' removed, I assume that 'profile-before-change' doesn't guarantee removal of cookies. So, right thing to do would be to modify the cleanse part of the test files or call RemoveAll() based on a condition may be.
That part of the test is testing something which no longer exists, and we should just remove that part of the test.
Latest try's at https://tbpl.mozilla.org/?tree=Try&rev=105f547fe7cf
Attachment #702922 - Attachment is obsolete: true
Mavericks, are you still working on this?
Assignee: kshriram18 → nobody
(In reply to Benjamin Smedberg [:bsmedberg] from comment #17) > Mavericks, are you still working on this? No. Apologies for the late reply. Someone else interested can take up this if it's still valid.
Can I please assigned for this bug?
Decheol, please feel free to create a patch for this bug. We typically don't assign bugs to new contributors until after they have submitted an initial patch.
Decheol Jeong, are you still planning on creating a patch for this? Or should I try and fix this bug instead?
I'am new to OPen source contribution. This is my first patch for this bug.
Attachment #8547166 - Flags: review?(benjamin)
Comment on attachment 8547166 [details] [diff] [review] Bug-820613-fix There is trailing whitespace in this patch. Please remove this whitespace before checkin.
Assignee: nobody → abhi.workspace
Status: NEW → ASSIGNED
Attachment #8547166 - Attachment is obsolete: true
(In reply to abhi.workspace from comment #24) > Created attachment 8548268 [details] [diff] [review] > checkin needed This patch is missing your name, commit message, etc. To find out how to add them, see: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F While you're at it, can you also remove the paragraph about shutdown-cleanse in profile/notifications.txt? : http://mxr.mozilla.org/mozilla-central/source/profile/notifications.txt#59
Attachment #8548268 - Attachment is obsolete: true
Seeing as how this got backed out previously for test failures, a link to a green Try run would be much-appreciated :)
Looks like this was removed by Ehsan over in bug 1116545 at almost exactly the same time as this bug was being worked on, oddly enough.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1116545
You need to log in before you can comment on or make changes to this bug.