Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(42)

Issue 557833006: Fix WeakPtrFactory member ordering in chrome/browser and chrome/service (Closed)

Created:
6 years, 3 months ago by MRV
Modified:
6 years, 3 months ago
Reviewers:
sky, marja
CC:
chromium-reviews, tim+watch_chromium.org, skanuj+watch_chromium.org, melevin+watch_chromium.org, zea+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, haitaol+watch_chromium.org, dominich, marja+watch_chromium.org, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered, maniscalco+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Fix WeakPtrFactory member ordering in chrome/browser Changing in the intialization order of WeakPtrFactory such that all member variables should appear before the WeakPtrFactory to ensure that any WeakPtrs to Controller are invalidated before its members variable's destructors are executed, rendering them invalid. BUG=303818 Committed: https://crrev.com/745953646fc11c8ae13620ec3ee561a0e44a8caf Cr-Commit-Position: refs/heads/master@{#296216}

Patch Set 1 #

Patch Set 2 : Add WeakPtr related comments #

Patch Set 3 : Removed the WeakPtr comments in files #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -18 lines) Patch
M chrome/browser/search/instant_service.h View 2 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/sessions/base_session_service.h View 2 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/sessions/base_session_service.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/sync/sessions/notification_service_sessions_router.h View 2 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/upgrade_detector_impl.h View 2 2 chunks +5 lines, -5 lines 0 comments Download
M chrome/browser/upgrade_detector_impl.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/service/cloud_print/printer_job_handler.h View 2 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 35 (9 generated)
MRV
PTAL
6 years, 3 months ago (2014-09-17 07:38:12 UTC) #2
marja
Drive-by comments: It would be useful to add comments near the WeakPtrFactory, e.g., SomeMemberVar var_; ...
6 years, 3 months ago (2014-09-17 07:43:00 UTC) #4
MRV
On 2014/09/17 07:43:00, marja wrote: > Drive-by comments: > > It would be useful to ...
6 years, 3 months ago (2014-09-17 08:46:42 UTC) #5
marja
On 2014/09/17 08:46:42, MRV wrote: > On 2014/09/17 07:43:00, marja wrote: > > Drive-by comments: ...
6 years, 3 months ago (2014-09-17 08:51:07 UTC) #6
sky
I commented as much in similar reviews. Really there needs to be a presubmit check, ...
6 years, 3 months ago (2014-09-17 15:47:36 UTC) #8
MRV
On 2014/09/17 15:47:36, sky wrote: > I commented as much in similar reviews. Really there ...
6 years, 3 months ago (2014-09-17 17:03:27 UTC) #9
MRV
On 2014/09/17 08:51:07, marja wrote: > On 2014/09/17 08:46:42, MRV wrote: > > On 2014/09/17 ...
6 years, 3 months ago (2014-09-18 03:28:46 UTC) #10
MRV
Dear Mr.Sky and Mr.Marja, please review my patch.
6 years, 3 months ago (2014-09-18 15:51:34 UTC) #11
MRV
Dear Mr.Sky and Mr.Marja, please review my patch.
6 years, 3 months ago (2014-09-18 15:51:34 UTC) #12
marja
(I think sky@ is the right person to review, since I *still* don't get this.) ...
6 years, 3 months ago (2014-09-18 16:04:42 UTC) #13
sky
Marja, search for the thread "WeakPtrFactory member variable order rule weakly followed". For the majority ...
6 years, 3 months ago (2014-09-18 19:31:35 UTC) #14
MRV
On 2014/09/18 19:31:35, sky wrote: > Marja, search for the thread "WeakPtrFactory member variable order ...
6 years, 3 months ago (2014-09-19 03:08:29 UTC) #15
MRV
On 2014/09/19 03:08:29, MRV wrote: > On 2014/09/18 19:31:35, sky wrote: > > Marja, search ...
6 years, 3 months ago (2014-09-23 01:59:42 UTC) #16
marja
(Sorry for responding so slowly, I'm somewhat confused why sky@ didn't yet l-g-t-m this, as ...
6 years, 3 months ago (2014-09-23 07:37:27 UTC) #18
MRV
On 2014/09/23 07:37:27, marja wrote: > (Sorry for responding so slowly, I'm somewhat confused why ...
6 years, 3 months ago (2014-09-23 08:44:09 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/557833006/40001
6 years, 3 months ago (2014-09-23 08:45:50 UTC) #21
MRV
Dear Mr. Sky, Could you please looks Review, looks like Mr. marja are author of ...
6 years, 3 months ago (2014-09-23 08:48:19 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/12789)
6 years, 3 months ago (2014-09-23 08:56:22 UTC) #24
sky
LGTM
6 years, 3 months ago (2014-09-23 13:21:24 UTC) #25
MRV
On 2014/09/23 13:21:24, sky wrote: > LGTM Thanks Mr. Sky for LGTM
6 years, 3 months ago (2014-09-23 13:38:28 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/557833006/40001
6 years, 3 months ago (2014-09-23 13:38:42 UTC) #28
commit-bot: I haz the power
Failed to apply the patch.
6 years, 3 months ago (2014-09-23 14:29:18 UTC) #30
marja
On 2014/09/23 08:44:09, MRV wrote: > On 2014/09/23 07:37:27, marja wrote: > > (Sorry for ...
6 years, 3 months ago (2014-09-23 14:55:21 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/557833006/40001
6 years, 3 months ago (2014-09-23 16:57:58 UTC) #33
commit-bot: I haz the power
Committed patchset #3 (id:40001) as c0bed6293c4b4465d8bc689af7df3bed9ccd9b0b
6 years, 3 months ago (2014-09-23 20:35:44 UTC) #34
commit-bot: I haz the power
6 years, 3 months ago (2014-09-23 20:36:19 UTC) #35
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/745953646fc11c8ae13620ec3ee561a0e44a8caf
Cr-Commit-Position: refs/heads/master@{#296216}

Powered by Google App Engine
This is Rietveld 408576698