Chromium Code Reviews
Help | Chromium Project | Sign in
(49)

Issue 404012: Ensure that the refcount on InternalGetCommandRequest stays non-zero through a PostTask (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 7 months ago by jamesr
Modified:
4 years ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Ensure that the refcount on InternalGetCommandRequest stays non-zero through a PostTask The problem was that BaseSessionService::ScheduleGetLastSessionCommands() was posting a task with a InternalGetCommandsRequest* request parameter by calling NewRunnableMethod(.., &SessionBackend::ReadLastSessionCommands, request). SessnionBackend::ReadLastSessionCommands takes one parameter of type scoped_refptr<InternalGetCommandsRequest>. However, NewRunnableMethod was matching the template because an InternalGetCommandsRequest* is implicitly convertable to a scoped_refptr<InternalGetCommandsRequest> but it was not actually creating the scoped_refptr<> (and thus bumping the refcount) until the task was dispatched. By this time the refcount on the InternalGetCommandsRequest had already dropped to zero, leading to memory corruption. This fixes the problem by passing a scoped_refptr<...> in to NewRunnableMethod() to ensure that it is copied and that the refcount stays up. TEST=covered by TabRestoreUITest.RestoreIntoSameWindow - caused very intermittend failures locally BUG=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=32240

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -1 line) Patch
M chrome/browser/sessions/base_session_service.cc View 1 chunk +1 line, -1 line 0 comments Download
Commit: CQ not working?

Messages

Total messages: 2 (0 generated)
jamesr
The failure reproduces much more reliably if the PostTask() is changed to a PostDelayedTask() with ...
5 years, 7 months ago (2009-11-17 23:53:43 UTC) #1
jam
5 years, 7 months ago (2009-11-18 00:10:13 UTC) #2
great catch.  that's crazy though.  can we prevent this from happening again,
either through compile/runtime checks?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1f9106d