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

Issue 2221513002: Remove another potential stale CJS_Timer usage (Closed)

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

Description

Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The |app| class owns the CJS_Timer as part of its vector<unique_ptr> to them. The CJS_Timer "owns" its slot in the global ID to timer map, and removes itself when it is destroyed. Nothing else deletes from the global map. Deleting from the global map is accompanied by a callback to the embedder to clear its resources. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which then deletes the CJS_Timer, which in turn cleans up the global map. Provide a CJS_Timer::Cancel static method to do this conveniently. There is a alternate path to the CJS_timer via JS and its CJS_TimerObj. CJS_TimerObj owns a TimerObj that currently points to the CJS_Timer. If the timer fires, and cleans itself up, this can go stale. Make the TimerObj maintain a weak reference via global timer ID rather than a direct pointer to the CJS_Timer, so that if the timer fires and is destroyed, future attempts to cancel find nothing. There is another path, where if the JS timer object is GC'd, then we just clean up its CJS_TimerObj without touching the actual CJS_Timers. We could make this match the spec by calling into the new cancel routine as described above, but it seems weird to have a timer depend on whether a gc happened or not. A subsequent CL will rename these objects to more closely match the conventions used by the other JS wrappers. BUG=634716 Committed: https://pdfium.googlesource.com/pdfium/+/8ca63de14d522d3d259d74fa43b28b05b02728e8

Patch Set 1 #

Patch Set 2 : rework, weak refs, single deletion path #

Total comments: 6

Patch Set 3 : add tests #

Patch Set 4 : Address comments #

Patch Set 5 : tidy pdf #

Unified diffs Side-by-side diffs Delta from patch set Stats (+242 lines, -76 lines) Patch
M fpdfsdk/fpdfformfill_embeddertest.cpp View 1 2 1 chunk +22 lines, -0 lines 0 comments Download
M fpdfsdk/javascript/JS_Object.h View 1 2 3 2 chunks +6 lines, -4 lines 0 comments Download
M fpdfsdk/javascript/JS_Object.cpp View 1 2 3 3 chunks +23 lines, -15 lines 0 comments Download
M fpdfsdk/javascript/app.h View 1 2 chunks +3 lines, -3 lines 0 comments Download
M fpdfsdk/javascript/app.cpp View 1 2 3 3 chunks +14 lines, -33 lines 0 comments Download
A testing/resources/bug_634716.in View 1 2 3 4 1 chunk +126 lines, -0 lines 0 comments Download
A + testing/resources/bug_634716.pdf View 1 2 3 4 2 chunks +48 lines, -21 lines 0 comments Download

Messages

Total messages: 30 (24 generated)
Tom Sepez
Lei, for review.
2 years, 10 months ago (2016-08-05 21:32:13 UTC) #8
Lei Zhang
Is it possible to write a new test for this case? https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/JS_Object.cpp File fpdfsdk/javascript/JS_Object.cpp (right): ...
2 years, 10 months ago (2016-08-05 22:29:56 UTC) #20
Tom Sepez
Ok, test trips on master/asan. https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/JS_Object.cpp File fpdfsdk/javascript/JS_Object.cpp (right): https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/JS_Object.cpp#newcode103 fpdfsdk/javascript/JS_Object.cpp:103: if (!m_nTimerID) On 2016/08/05 ...
2 years, 10 months ago (2016-08-05 23:11:42 UTC) #21
Lei Zhang
lgtm
2 years, 10 months ago (2016-08-06 00:02:17 UTC) #26
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/2221513002/80001
2 years, 10 months ago (2016-08-06 00:12:12 UTC) #28
commit-bot: I haz the power
2 years, 10 months ago (2016-08-06 00:12:31 UTC) #30
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://pdfium.googlesource.com/pdfium/+/8ca63de14d522d3d259d74fa43b28b05b027...

Powered by Google App Engine
This is Rietveld 408576698