Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2221513002:
Remove another potential stale CJS_Timer usage (Closed)
DescriptionRemove 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 #
Messages
Total messages: 30 (24 generated)
Description was changed from ========== Remove another potential stale CJS_Timer usage BUG=634716 ========== to ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app owns the CJS_Timer. The CJS_Timer owns its slot in the global map, and removes itself when it is destroyed. Next, the only way to remove a CJS_Timer is by going through the app. The alternate path to the timer is from JS, through the CJS_TimerObj/TimerObj objects. Make these maintain a weak reference via timer ID rather than a direct pointer to the timer, so that if the timer fires and is destroyed, future attempts to cancel find nothing. BUG=634716 ==========
tsepez@chromium.org changed reviewers: + thestig@chromium.org
Description was changed from ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app owns the CJS_Timer. The CJS_Timer owns its slot in the global map, and removes itself when it is destroyed. Next, the only way to remove a CJS_Timer is by going through the app. The alternate path to the timer is from JS, through the CJS_TimerObj/TimerObj objects. Make these maintain a weak reference via timer ID rather than a direct pointer to the timer, so that if the timer fires and is destroyed, future attempts to cancel find nothing. BUG=634716 ========== to ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app owns the CJS_Timer as part of its vector<unique_ptr> to them. The CJS_Timer "owns" its slot in the global map, and removes itself when it is destroyed. Nothing else deletes from the global map. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in turn cleans up the global map. 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. BUG=634716 ==========
Description was changed from ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app owns the CJS_Timer as part of its vector<unique_ptr> to them. The CJS_Timer "owns" its slot in the global map, and removes itself when it is destroyed. Nothing else deletes from the global map. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in turn cleans up the global map. 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. BUG=634716 ========== to ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app owns the CJS_Timer as part of its vector<unique_ptr> to them. The CJS_Timer "owns" its slot in the global map, and removes itself when it is destroyed. Nothing else deletes from the global map. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in turn cleans up the global map. 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. A subsequent CL will rename these objects to more closely match the conventions used by the other JS wrappers. BUG=634716 ==========
Description was changed from ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app owns the CJS_Timer as part of its vector<unique_ptr> to them. The CJS_Timer "owns" its slot in the global map, and removes itself when it is destroyed. Nothing else deletes from the global map. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in turn cleans up the global map. 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. A subsequent CL will rename these objects to more closely match the conventions used by the other JS wrappers. BUG=634716 ========== to ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app owns the CJS_Timer as part of its vector<unique_ptr> to them. The CJS_Timer "owns" its slot in the global map, and removes itself when it is destroyed. Nothing else deletes from the global map. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in deletes the CJS_Timer, which in turn cleans up the global map. 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. A subsequent CL will rename these objects to more closely match the conventions used by the other JS wrappers. BUG=634716 ==========
Description was changed from ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app owns the CJS_Timer as part of its vector<unique_ptr> to them. The CJS_Timer "owns" its slot in the global map, and removes itself when it is destroyed. Nothing else deletes from the global map. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in deletes the CJS_Timer, which in turn cleans up the global map. 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. A subsequent CL will rename these objects to more closely match the conventions used by the other JS wrappers. BUG=634716 ========== to ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app owns the CJS_Timer as part of its vector<unique_ptr> to them. The CJS_Timer "owns" its slot in the global map, and removes itself when it is destroyed. Nothing else deletes from the global map. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in deletes the CJS_Timer, which in turn cleans up the global map. 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. A subsequent CL will rename these objects to more closely match the conventions used by the other JS wrappers. BUG=634716 ==========
Description was changed from ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app owns the CJS_Timer as part of its vector<unique_ptr> to them. The CJS_Timer "owns" its slot in the global map, and removes itself when it is destroyed. Nothing else deletes from the global map. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in deletes the CJS_Timer, which in turn cleans up the global map. 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. A subsequent CL will rename these objects to more closely match the conventions used by the other JS wrappers. BUG=634716 ========== to ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app 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. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in deletes the CJS_Timer, which in turn cleans up the global map. 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. A subsequent CL will rename these objects to more closely match the conventions used by the other JS wrappers. BUG=634716 ==========
Lei, for review.
The CQ bit was checked by tsepez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app 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. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in deletes the CJS_Timer, which in turn cleans up the global map. 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. A subsequent CL will rename these objects to more closely match the conventions used by the other JS wrappers. BUG=634716 ========== to ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app 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. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in 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 objects 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 happend or not. A subsequent CL will rename these objects to more closely match the conventions used by the other JS wrappers. BUG=634716 ==========
Description was changed from ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app 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. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in 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 objects 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 happend or not. A subsequent CL will rename these objects to more closely match the conventions used by the other JS wrappers. BUG=634716 ========== to ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app 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. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in 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 objects 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 ==========
Description was changed from ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app 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. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in 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 objects 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 ========== to ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app 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. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in 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 ==========
Description was changed from ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app 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. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in 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 ========== to ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app 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. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in 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 ==========
Description was changed from ========== Remove another potential stale CJS_Timer usage Fix memory ownership model for PDFium timers. The app 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. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in 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 ========== to ========== 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. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in 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 ==========
Description was changed from ========== 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. Next, the proper way to remove a CJS_Timer is by going through the app, and having the app erase its unique ptr, which in 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 ========== to ========== 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. 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== 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. 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 ========== to ========== 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 ==========
Is it possible to write a new test for this case? https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/JS_O... File fpdfsdk/javascript/JS_Object.cpp (right): https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/JS_O... fpdfsdk/javascript/JS_Object.cpp:103: if (!m_nTimerID) I wonder if this can eval to true. https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/JS_O... File fpdfsdk/javascript/JS_Object.h (right): https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/JS_O... fpdfsdk/javascript/JS_Object.h:84: bool IsOneTime() const { return m_nType == 1; } Maybe IsOneShot() ? https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/app.cpp File fpdfsdk/javascript/app.cpp (right): https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/app.... fpdfsdk/javascript/app.cpp:38: void TimerObj::SetTimer(CJS_Timer* pTimer) { Maybe just take the ID?
Ok, test trips on master/asan. https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/JS_O... File fpdfsdk/javascript/JS_Object.cpp (right): https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/JS_O... fpdfsdk/javascript/JS_Object.cpp:103: if (!m_nTimerID) On 2016/08/05 22:29:54, Lei Zhang wrote: > I wonder if this can eval to true. Probably not. The CTOR sets this to zero, so if we branch out before calling settimer it could happen. Except we don't do that. I'll leave it for now. https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/JS_O... File fpdfsdk/javascript/JS_Object.h (right): https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/JS_O... fpdfsdk/javascript/JS_Object.h:84: bool IsOneTime() const { return m_nType == 1; } On 2016/08/05 22:29:55, Lei Zhang wrote: > Maybe IsOneShot() ? Done. https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/app.cpp File fpdfsdk/javascript/app.cpp (right): https://codereview.chromium.org/2221513002/diff/20001/fpdfsdk/javascript/app.... fpdfsdk/javascript/app.cpp:38: void TimerObj::SetTimer(CJS_Timer* pTimer) { On 2016/08/05 22:29:55, Lei Zhang wrote: > Maybe just take the ID? I wanted to prove that you weren't just making up an ID out of thin air, that you actually had the object with the id at one point.
The CQ bit was checked by tsepez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/8ca63de14d522d3d259d74fa43b28b05b027... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://pdfium.googlesource.com/pdfium/+/8ca63de14d522d3d259d74fa43b28b05b027... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
