Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 2179283005: Remove pageview from map immediately (Closed)

Created:
2 years, 9 months ago by dsinclair
Modified:
2 years, 9 months ago
Reviewers:
Lei Zhang, Wei Li
CC:
pdfium-reviews_googlegroups.com, npm
Base URL:
https://pdfium.googlesource.com/pdfium.git@master
Target Ref:
refs/heads/master
Project:
pdfium
Visibility:
Public.

Description

Remove pageview from map immediately There seems to be an ownership issue in the page annotation code where removing the annotations can result in removing the parent page view. This is fine except that removing the parent page view removes the annotations and you can end up with a use-after-free. This CL removes the page view from the documents page map immediately and then proceeds with the cleanup. Then, if we try to remove that page again it won't be found and we won't double free. BUG=chromium:630654 Committed: https://pdfium.googlesource.com/pdfium/+/49dce65dc78bcd5a0c78a8bbdf2809cf20212220

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -1 line) Patch
M fpdfsdk/fsdk_mgr.cpp View 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 13 (7 generated)
dsinclair
PTAL. I'm going to see if I can track down why removing the annotation frees ...
2 years, 9 months ago (2016-07-26 18:07:18 UTC) #4
Lei Zhang
lgtm BTW, no need for chromium: prefix.
2 years, 9 months ago (2016-07-26 18:46:45 UTC) #7
dsinclair
On 2016/07/26 18:46:45, Lei Zhang wrote: > lgtm > > BTW, no need for chromium: ...
2 years, 9 months ago (2016-07-26 19:09:14 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2179283005/1
2 years, 9 months ago (2016-07-26 19:09:28 UTC) #10
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://pdfium.googlesource.com/pdfium/+/49dce65dc78bcd5a0c78a8bbdf2809cf20212220
2 years, 9 months ago (2016-07-26 19:09:47 UTC) #12
dsinclair
2 years, 9 months ago (2016-07-26 19:19:35 UTC) #13
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2188523002/ by dsinclair@chromium.org.

The reason for reverting is: Looks like this broke linux_asan embedder tests.

https://build.chromium.org/p/client.pdfium/builders/linux_asan/builds/1152/st....

Powered by Google App Engine
This is Rietveld 408576698