Closed Bug 820613 Opened 7 years ago Closed 5 years ago

Remove all references to "shutdown-cleanse"

Categories

(Toolkit :: Startup and Profile System, defect, trivial)

defect
Not set
trivial

Tracking

()

RESOLVED DUPLICATE of bug 1116545

People

(Reporter: briansmith, Assigned: abhi.workspace, Mentored)

References

Details

(Whiteboard: [good first bug][lang=C++/JS])

Attachments

(1 file, 6 obsolete files)

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
 ?
Blocks: 791546
No longer blocks: 791546
Updated patch and to tip
Attachment #695327 - Attachment is obsolete: true
Attachment #695409 - Flags: review?
Attachment #695409 - Flags: review? → review?(benjamin)
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)
Attachment #695409 - Attachment is obsolete: true
Attachment #702922 - Flags: review?(benjamin)
Blocks: 791546
Attachment #702922 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2f0c1fffc2
Assignee: nobody → kshriram18
Flags: in-testsuite+
Keywords: checkin-needed
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://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/kshriram18@gmail.com-c93a65e68983
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://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/kshriram18@gmail.com-99b8a9a3b478
Duplicate of this bug: 846760
Duplicate of this bug: 846764
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.
Flags: needinfo?
That part of the test is testing something which no longer exists, and we should just remove that part of the test.
Flags: needinfo?
Mavericks, are you still working on this?
Flags: needinfo?(kshriram18)
Assignee: kshriram18 → nobody
Flags: needinfo?(kshriram18)
(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.
Mentor: benjamin
Whiteboard: [good first bug][lang=C++/JS]
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?
Attached patch Bug-820613-fix (obsolete) — Splinter Review
I'am new to OPen source contribution. This is my first patch for this bug.
Attachment #8547166 - Flags: review?(benjamin)
Attachment #8547166 - Flags: review?(benjamin) → review+
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
Attached patch checkin needed (obsolete) — Splinter Review
Attachment #8548268 - Flags: checkin?
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[0]?

[0]: http://mxr.mozilla.org/mozilla-central/source/profile/notifications.txt#59
Keywords: checkin-needed
Attached patch Bug-820613-fixSplinter Review
checkin needed
Attachment #8548325 - Flags: checkin?
Keywords: checkin-needed
Attachment #723886 - Attachment is obsolete: true
Attachment #8548268 - Attachment is obsolete: true
Attachment #8548325 - Flags: checkin?
Seeing as how this got backed out previously for test failures, a link to a green Try run would be much-appreciated :)
Keywords: checkin-needed
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
No longer blocks: 791546
You need to log in before you can comment on or make changes to this bug.