Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2649513002:
Add a pre-finalizer for XHR to cancel the loader (Closed)
DescriptionAdd a pre-finalizer for XHR to cancel the loader
In some cases hasPendingActivity doesn't work correctly, i.e., doesn't keep
|this| alive. We need to cancel the loader in such cases, so this CL adds
a pre-finalizer.
BUG=667254
Review-Url: https://codereview.chromium.org/2649513002
Cr-Commit-Position: refs/heads/master@{#445018}
Committed: https://chromium.googlesource.com/chromium/src/+/a05ab964ff708f4edcfc38f90b5f933f7a07478b
Patch Set 1 #
Total comments: 6
Messages
Total messages: 25 (9 generated)
The CQ bit was checked by yhirano@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...
yhirano@chromium.org changed reviewers: + kouhei@chromium.org, tyoshino@chromium.org
kouhei@, is this what you meant? Is there a filed bug for the hasPendingActivity issue?
lgtm I have some concern with that we're not implementing the logic we have in internalAbort(), this does improve the situation, I guess.
lgtm for last resort solution yes, but I'd like to check w/ shiino-san
kouhei@chromium.org changed reviewers: + yukishiino@chromium.org
+shiinosan
+yukishiino@
On 2017/01/20 06:29:54, tyoshino wrote: > lgtm > > I have some concern with that we're not implementing the logic we have in > internalAbort(), this does improve the situation, I guess. Regarding this point, filed http://crbug.com/683000. Thanks yhirano for investigation.
LGTM. https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h (right): https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h:76: // In some cases hasPendingActivity doesn't work correctly, i.e., IMHO, hasPendingActivity is not designed to keep the object alive forever or unconditionally. Even if an object has a pending task, we don't need to keep the object or its wrapper if the context is discarded for example. The spec says that we should discard queued tasks if the context is discarded, but probably we're not doing it correctly, and it could be the root cause.
https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h (right): https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h:76: // In some cases hasPendingActivity doesn't work correctly, i.e., On 2017/01/20 07:02:53, Yuki wrote: > IMHO, hasPendingActivity is not designed to keep the object alive forever or > unconditionally. Even if an object has a pending task, we don't need to keep > the object or its wrapper if the context is discarded for example. > > The spec says that we should discard queued tasks if the context is discarded, > but probably we're not doing it correctly, and it could be the root cause. We have contextDestroyed and it makes hasPendingActivity return false.
The CQ bit was unchecked by yhirano@chromium.org
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1484898940797430, "parent_rev":
"f24aa8ad5db849c114c91cddcc297475b8ead2c8", "commit_rev":
"a05ab964ff708f4edcfc38f90b5f933f7a07478b"}
Message was sent while issue was closed.
Description was changed from ========== Add a pre-finalizer for XHR to cancel the loader In some cases hasPendingActivity doesn't work correctly, i.e., doesn't keep |this| alive. We need to cancel the loader in such cases, so this CL adds a pre-finalizer. BUG=667254 ========== to ========== Add a pre-finalizer for XHR to cancel the loader In some cases hasPendingActivity doesn't work correctly, i.e., doesn't keep |this| alive. We need to cancel the loader in such cases, so this CL adds a pre-finalizer. BUG=667254 Review-Url: https://codereview.chromium.org/2649513002 Cr-Commit-Position: refs/heads/master@{#445018} Committed: https://chromium.googlesource.com/chromium/src/+/a05ab964ff708f4edcfc38f90b5f... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/a05ab964ff708f4edcfc38f90b5f...
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
(i'd appreciate being looped in on CLs in this general area.) https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h (right): https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h:80: USING_PRE_FINALIZER(XMLHttpRequest, dispose); This object is already eagerly finalized, so also adding a prefinalizer is a bit strange. Actually, I wonder if moving ThreadableLoader to the heap introduced an "issue" here, as the eager finalization would have let go of the loader promptly before, but it now doesn't. Assuming it is a bug for the loader and the XHR object to quietly die during the same sweep, we can either stick with eager finalization or a prefinalizer, but not both. I'm fine with using a prefinalizer, as the number of live XHRs wouldn't be excessively large. I hope cancel() won't touch other prefinalized objects.
Message was sent while issue was closed.
https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h (right): https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h:80: USING_PRE_FINALIZER(XMLHttpRequest, dispose); On 2017/01/20 09:17:23, sof wrote: > This object is already eagerly finalized, so also adding a prefinalizer is a bit > strange. > > Actually, I wonder if moving ThreadableLoader to the heap introduced an "issue" > here, as the eager finalization would have let go of the loader promptly before, > but it now doesn't. > > Assuming it is a bug for the loader and the XHR object to quietly die during the > same sweep, No. As written in the ThreadableLoader comments, a ThreadableLoader must be completed (including canceled) before its destruction. Quietly dying is a problem and the sweeping order is not related. I thought that rule is kept via hasPendingActivity and contextDestroyed but I heard otherwise today. I would like to it a "bug" of ActiveScriptWrappable or ContextLifecycleObserver so I added the TODO. > we can either stick with eager finalization or a prefinalizer, but > not both. I'm fine with using a prefinalizer, as the number of live XHRs > wouldn't be excessively large. Thanks, I will stop XHR from EAGERLY_FINALIZing next week. > I hope cancel() won't touch other prefinalized objects. DocumentThreadableLoader::cancel calls Resource::removeClient which can be called in the pre-finalization step. It also calls ThreadableLoaderClient::didFail, but with m_error set it does nothing.
Message was sent while issue was closed.
https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h (right): https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h:80: USING_PRE_FINALIZER(XMLHttpRequest, dispose); On 2017/01/20 09:49:56, yhirano wrote: > On 2017/01/20 09:17:23, sof wrote: > > we can either stick with eager finalization or a prefinalizer, but > > not both. I'm fine with using a prefinalizer, as the number of live XHRs > > wouldn't be excessively large. > Thanks, I will stop XHR from EAGERLY_FINALIZing next week. > What happens if we specify both? 1) Something very bad happens (e.g., crash, leak) 2) One of them is ignored 3) They're working but wasteful
Message was sent while issue was closed.
https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h (right): https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h:80: USING_PRE_FINALIZER(XMLHttpRequest, dispose); On 2017/01/20 09:54:08, yhirano wrote: > On 2017/01/20 09:49:56, yhirano wrote: > > On 2017/01/20 09:17:23, sof wrote: > > > we can either stick with eager finalization or a prefinalizer, but > > > not both. I'm fine with using a prefinalizer, as the number of live XHRs > > > wouldn't be excessively large. > > Thanks, I will stop XHR from EAGERLY_FINALIZing next week. > > > What happens if we specify both? > > 1) Something very bad happens (e.g., crash, leak) > 2) One of them is ignored > 3) They're working but wasteful > 3, it is confused and wasteful to do both. The destructor must run if eagerly finalized, so you get to promptly let go of some resources, like some of the buffers and decoder objects that XHR has.
Message was sent while issue was closed.
On 2017/01/20 09:57:45, sof wrote: > https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h (right): > > https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h:80: > USING_PRE_FINALIZER(XMLHttpRequest, dispose); > On 2017/01/20 09:54:08, yhirano wrote: > > On 2017/01/20 09:49:56, yhirano wrote: > > > On 2017/01/20 09:17:23, sof wrote: > > > > we can either stick with eager finalization or a prefinalizer, but > > > > not both. I'm fine with using a prefinalizer, as the number of live XHRs > > > > wouldn't be excessively large. > > > Thanks, I will stop XHR from EAGERLY_FINALIZing next week. > > > > > What happens if we specify both? > > > > 1) Something very bad happens (e.g., crash, leak) > > 2) One of them is ignored > > 3) They're working but wasteful > > > > 3, it is confused and wasteful to do both. The destructor must run if eagerly > finalized, so you get to promptly let go of some resources, like some of the > buffers and decoder objects that XHR has. Do you think this is related to the leak of fetch-request-redirect.https.html? https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty...
Message was sent while issue was closed.
On 2017/01/20 10:42:53, haraken wrote: > On 2017/01/20 09:57:45, sof wrote: > > > https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h (right): > > > > > https://codereview.chromium.org/2649513002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h:80: > > USING_PRE_FINALIZER(XMLHttpRequest, dispose); > > On 2017/01/20 09:54:08, yhirano wrote: > > > On 2017/01/20 09:49:56, yhirano wrote: > > > > On 2017/01/20 09:17:23, sof wrote: > > > > > we can either stick with eager finalization or a prefinalizer, but > > > > > not both. I'm fine with using a prefinalizer, as the number of live XHRs > > > > > wouldn't be excessively large. > > > > Thanks, I will stop XHR from EAGERLY_FINALIZing next week. > > > > > > > What happens if we specify both? > > > > > > 1) Something very bad happens (e.g., crash, leak) > > > 2) One of them is ignored > > > 3) They're working but wasteful > > > > > > > 3, it is confused and wasteful to do both. The destructor must run if eagerly > > finalized, so you get to promptly let go of some resources, like some of the > > buffers and decoder objects that XHR has. > > Do you think this is related to the leak of fetch-request-redirect.https.html? > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty... Don't think so, looks like https://codereview.chromium.org/2640983005 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
