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

Issue 1417063006: Revert of Call reset(nullptr) in scoped_ptr's. (Closed)

Created:
5 years, 1 month ago by perkj_chrome
Modified:
5 years, 1 month ago
CC:
chromium-reviews, gavinp+memory_chromium.org, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Revert of Call reset(nullptr) in scoped_ptr's. (patchset #7 id:120001 of https://codereview.chromium.org/1356483002/ ) Reason for revert: Sorry for reverting, I assume there is nothing really wrong with this cl but it breaks ToolbarControllerTest on Mac 10.10 and 10.11. http://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/3607 Original issue's description: > Call reset(nullptr) in scoped_ptr's destructor. > > This behaviour is consistent with libc++. > > The original goal of this change is to make Chrome crash more often > and closer to the source when there's a use-after-free bug that > accesses a destroyed scoped_ptr. > > BUG=535321 > > Committed: https://crrev.com/cf9db6ef148d1493dbf6bdc6693f9c79e3ac2afc > Cr-Commit-Position: refs/heads/master@{#357297} TBR=danakj@chromium.org,pkasting@chromium.org,amistry@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=535321 Committed: https://crrev.com/0db76eefa32b0b4c29d4a01c1849ac2b2feecb15 Cr-Commit-Position: refs/heads/master@{#357335}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -23 lines) Patch
M base/memory/scoped_ptr.h View 1 chunk +5 lines, -11 lines 0 comments Download
M base/memory/scoped_ptr_unittest.cc View 1 chunk +2 lines, -12 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
perkj_chrome
Created Revert of Call reset(nullptr) in scoped_ptr's.
5 years, 1 month ago (2015-11-02 11:50:05 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417063006/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417063006/1
5 years, 1 month ago (2015-11-02 11:50:14 UTC) #2
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 1 month ago (2015-11-02 11:50:46 UTC) #3
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/0db76eefa32b0b4c29d4a01c1849ac2b2feecb15 Cr-Commit-Position: refs/heads/master@{#357335}
5 years, 1 month ago (2015-11-02 11:51:44 UTC) #4
Peter Kasting
If this breaks that test, doesn't that mean the test is incorrect and should be ...
5 years, 1 month ago (2015-11-02 19:07:25 UTC) #5
danakj
5 years, 1 month ago (2015-11-02 19:41:06 UTC) #6
Message was sent while issue was closed.
On Mon, Nov 2, 2015 at 11:07 AM, <pkasting@chromium.org> wrote:

> If this breaks that test, doesn't that mean the test is incorrect and
> should be
> disabled until it's fixed, instead of reverting this?
>

It does, but there were a number of bugs that had to be fixed to land this,
I guess that one got missed. We can fix the test and then reland.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Powered by Google App Engine
This is Rietveld 408576698