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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 15686007: Remove Activity Log usage of Extension objects (Closed)

Created:
7 years, 6 months ago by felt
Modified:
7 years, 6 months ago
CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org, Mattias Nissler (ping if slow)
Visibility:
Public.

Description

Remove Activity Log usage of Extension objects Previously, the AL used Extension objects when notifying the UI of new activity. If an extension were uninstalled in the middle of logging something, the Extension object would be deallocated before the UI tried to use the reference. Since we are replacing the old UI, I am switching to using IDs instead of Extension objects. [[[ Reverted because it was causing heapcheck failures. Retrying a new version. ]]] BUG=236395 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203218 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203618 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203950 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204054

Patch Set 1 #

Patch Set 2 : Made TODO wording more clear #

Total comments: 4

Patch Set 3 : Rebased, fixed nits #

Patch Set 4 : Fully removed use of Extension obj from chrome_render_message_filter.cc #

Patch Set 5 : Removed an unused object #

Patch Set 6 : Switched to using TestingProfile to see if that helps with memory issues #

Patch Set 7 : Rebased #

Patch Set 8 : Rebase squashed some changes; fixed #

Patch Set 9 : Threading issue on mac trybot #

Patch Set 10 : Added removed code back to see if that fixes persistent memory errors #

Unified diffs Side-by-side diffs Delta from patch set Stats (+121 lines, -244 lines) Patch
M chrome/browser/extensions/activity_log/activity_log.h View 1 2 3 4 5 6 7 8 chunks +9 lines, -29 lines 0 comments Download
M chrome/browser/extensions/activity_log/activity_log.cc View 1 2 3 4 5 6 7 13 chunks +21 lines, -91 lines 0 comments Download
M chrome/browser/extensions/activity_log/activity_log_unittest.cc View 1 2 3 4 5 6 7 8 9 9 chunks +48 lines, -47 lines 0 comments Download
M chrome/browser/extensions/api/web_request/web_request_api.cc View 1 2 3 4 5 6 7 1 chunk +6 lines, -22 lines 0 comments Download
M chrome/browser/extensions/event_router.cc View 1 2 7 1 chunk +2 lines, -19 lines 0 comments Download
M chrome/browser/extensions/extension_function_dispatcher.cc View 1 2 3 4 5 6 7 10 chunks +15 lines, -14 lines 0 comments Download
M chrome/browser/renderer_host/chrome_render_message_filter.cc View 1 2 3 4 5 6 7 10 chunks +20 lines, -22 lines 0 comments Download

Messages

Total messages: 25 (0 generated)
felt
Hi Matt, can you please review this? It does the bugfix I mentioned in the ...
7 years, 6 months ago (2013-05-29 08:55:36 UTC) #1
Matt Perry
LGTM with small nits https://codereview.chromium.org/15686007/diff/7001/chrome/browser/extensions/activity_log/activity_log.h File chrome/browser/extensions/activity_log/activity_log.h (right): https://codereview.chromium.org/15686007/diff/7001/chrome/browser/extensions/activity_log/activity_log.h#newcode70 chrome/browser/extensions/activity_log/activity_log.h:70: void AddObserver(const Extension* extension, no ...
7 years, 6 months ago (2013-05-29 18:01:17 UTC) #2
felt
https://codereview.chromium.org/15686007/diff/7001/chrome/browser/extensions/activity_log/activity_log.h File chrome/browser/extensions/activity_log/activity_log.h (right): https://codereview.chromium.org/15686007/diff/7001/chrome/browser/extensions/activity_log/activity_log.h#newcode70 chrome/browser/extensions/activity_log/activity_log.h:70: void AddObserver(const Extension* extension, Now that the other CL ...
7 years, 6 months ago (2013-05-30 07:21:02 UTC) #3
felt
Hi jhawkins, can you please review this tiny change to chrome_render_message_filter.cc? Thanks.
7 years, 6 months ago (2013-05-30 07:23:19 UTC) #4
James Hawkins
lgtm
7 years, 6 months ago (2013-05-30 16:34:58 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/15686007/24001
7 years, 6 months ago (2013-05-30 16:42:16 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/15686007/24001
7 years, 6 months ago (2013-05-30 16:43:24 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/15686007/24001
7 years, 6 months ago (2013-05-30 17:47:02 UTC) #8
commit-bot: I haz the power
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=120166
7 years, 6 months ago (2013-05-30 19:02:27 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/15686007/24001
7 years, 6 months ago (2013-05-30 20:08:15 UTC) #10
commit-bot: I haz the power
Change committed as 203218
7 years, 6 months ago (2013-05-30 20:56:28 UTC) #11
Mattias Nissler (ping if slow)
FWIW, after this change heapcheck reports leaks with "Activity" in them like crazy: http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Heapcheck/builds/27156/steps/heapcheck%20test%3A%20unit/logs/stdio
7 years, 6 months ago (2013-05-31 14:16:07 UTC) #12
felt
I had thought this was being caused by a CL that shess landed at the ...
7 years, 6 months ago (2013-05-31 17:28:14 UTC) #13
felt
On 2013/05/31 17:28:14, felt wrote: > I had thought this was being caused by a ...
7 years, 6 months ago (2013-05-31 21:46:18 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/15686007/46001
7 years, 6 months ago (2013-06-01 03:44:26 UTC) #15
commit-bot: I haz the power
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=120888
7 years, 6 months ago (2013-06-01 05:41:00 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/15686007/46001
7 years, 6 months ago (2013-06-01 06:28:55 UTC) #17
commit-bot: I haz the power
Change committed as 203618
7 years, 6 months ago (2013-06-02 19:27:21 UTC) #18
felt
Made another small change to the unit test. Committing again in the hopes that this ...
7 years, 6 months ago (2013-06-04 00:10:02 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/15686007/63008
7 years, 6 months ago (2013-06-04 00:10:56 UTC) #20
commit-bot: I haz the power
Retried try job too often on linux_chromeos for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=121312
7 years, 6 months ago (2013-06-04 01:09:28 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/15686007/75001
7 years, 6 months ago (2013-06-04 04:44:16 UTC) #22
commit-bot: I haz the power
Change committed as 203950
7 years, 6 months ago (2013-06-04 13:03:06 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/15686007/98001
7 years, 6 months ago (2013-06-04 15:45:18 UTC) #24
commit-bot: I haz the power
7 years, 6 months ago (2013-06-04 21:55:15 UTC) #25
Message was sent while issue was closed. 
Change committed as 204054

Powered by Google App Engine
This is Rietveld 408576698