Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2622943002:
Fix WebVR requestAnimationFrame leaking (Closed)
|
Created:
3 years, 11 months ago by bsheedy Modified:
3 years, 11 months ago CC:
chromium-reviews, shans, rjwright, blink-reviews-animation_chromium.org, feature-vr-reviews_chromium.org, darktears, blink-reviews, Eric Willigers Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix WebVR requestAnimationFrame leaking
Manually clear the Member<ScriptedAnimationController> handle in VRDisplay like Document does in order to prevent leaks when the Document is destroyed and there are still outstanding requestAnimationFrame callbacks.
Re-add the two layout tests that were failing due to detecting the leaks.
BUG=679401
Review-Url: https://codereview.chromium.org/2622943002
Cr-Commit-Position: refs/heads/master@{#443711}
Committed: https://chromium.googlesource.com/chromium/src/+/d8028087727891a6575e8209daecf58035a95723
Patch Set 1 #Patch Set 2 : Rename pending callbacks vector #Patch Set 3 : Properly fix leak #
Total comments: 5
Patch Set 4 : Added comments to tests explaining why they end with outstanding callbacks #Patch Set 5 : Rebase to see if CQ failure fixed #
Messages
Total messages: 42 (13 generated)
bsheedy@chromium.org changed reviewers: + bajones@chromium.org
PTAL
mthiesse@chromium.org changed reviewers: + mthiesse@chromium.org
I'm not sure this is the right approach, shouldn't ScriptedAnimationController take care of cleaning up any remaining callbacks when it gets deleted?
On 2017/01/10 18:04:28, mthiesse wrote: > I'm not sure this is the right approach, shouldn't ScriptedAnimationController > take care of cleaning up any remaining callbacks when it gets deleted? That also seems like the correct approach to me, but I'm not seeing anything in either ScriptedAnimationController or its FrameRequestCallbackCollection that clean up on destruction. Considering those seem pretty integral to WebKit, I'd be hesitant changing their behavior just for WebVR - if cleaning up on destruction was desirable, I imagine it would have been added already.
On 2017/01/10 18:13:31, bsheedy wrote: > On 2017/01/10 18:04:28, mthiesse wrote: > > I'm not sure this is the right approach, shouldn't ScriptedAnimationController > > take care of cleaning up any remaining callbacks when it gets deleted? > > That also seems like the correct approach to me, but I'm not seeing anything in > either ScriptedAnimationController or its FrameRequestCallbackCollection that > clean up on destruction. Considering those seem pretty integral to WebKit, I'd > be hesitant changing their behavior just for WebVR - if cleaning up on > destruction was desirable, I imagine it would have been added already. I'm curious: If you re-write the tests that exposed this issue to use window.rAF instead does it still leak? I'd think other tests may have encountered that already if it were the case. If not we should figure out exactly how the normal rAF is cleaning up an mimic it. (And if not this approach seems fine but we should probably raise the leak issue with a webkit owner)
On 2017/01/10 20:11:23, bajones wrote: > On 2017/01/10 18:13:31, bsheedy wrote: > > On 2017/01/10 18:04:28, mthiesse wrote: > > > I'm not sure this is the right approach, shouldn't > ScriptedAnimationController > > > take care of cleaning up any remaining callbacks when it gets deleted? > > > > That also seems like the correct approach to me, but I'm not seeing anything > in > > either ScriptedAnimationController or its FrameRequestCallbackCollection that > > clean up on destruction. Considering those seem pretty integral to WebKit, I'd > > be hesitant changing their behavior just for WebVR - if cleaning up on > > destruction was desirable, I imagine it would have been added already. > > I'm curious: If you re-write the tests that exposed this issue to use window.rAF > instead does it still leak? I'd think other tests may have encountered that > already if it were the case. If not we should figure out exactly how the normal > rAF is cleaning up an mimic it. (And if not this approach seems fine but we > should probably raise the leak issue with a webkit owner) Using window.rAF doesn't cause the test to leak. I'll take a look at how they clean that up.
Took at look at window.rAF, and it seems to just call document.rAF, which in turn uses ScriptedAnimationController just like us (without any additional cleanup or anything). There is a minor difference in the ensureScriptedAnimationController function, which suspends the SAC if the page is in the background, but adding that to VRDisplay's eSAC doesn't do anything. +bokan@, who's an OWNER of core/frame (and thus LocalDOMWindow). Do you have any idea why WebVR's requestAnimationFrame can cause leaks but window's doesn't?
Description was changed from ========== Fix WebVR requestAnimationFrame leaking VRDisplay's requestAnimationFrame now keeps track of any pending callbacks and cancels them when the display's context is destroyed. This prevents leaking that was occuring if a test finished while there were still pending callbacks. Re-add the two layout tests that were failing due to detecting the leaks. BUG=679401 ========== to ========== Fix WebVR requestAnimationFrame leaking VRDisplay's requestAnimationFrame now keeps track of any pending callbacks and cancels them when the display's context is destroyed. This prevents leaking that was occuring if a test finished while there were still pending callbacks. Re-add the two layout tests that were failing due to detecting the leaks. BUG=679401 ==========
bsheedy@chromium.org changed reviewers: + bokan@chromium.org
Talked to some people on the Oilpan team and got the leak properly fixed with their help. Apparently, Document does actually do extra cleanup (m_scriptedAnimationController.clear()), but it's in shutdown(), not the destructor like I initially thought.
On 2017/01/12 18:09:08, bsheedy wrote: > Talked to some people on the Oilpan team and got the leak properly fixed with > their help. Apparently, Document does actually do extra cleanup > (m_scriptedAnimationController.clear()), but it's in shutdown(), not the > destructor like I initially thought. LGTM
On 2017/01/10 22:23:11, bsheedy wrote: > Took at look at window.rAF, and it seems to just call document.rAF, which in > turn uses ScriptedAnimationController just like us (without any additional > cleanup or anything). There is a minor difference in the > ensureScriptedAnimationController function, which suspends the SAC if the page > is in the background, but adding that to VRDisplay's eSAC doesn't do anything. > > +bokan@, who's an OWNER of core/frame (and thus LocalDOMWindow). Do you have any > idea why WebVR's requestAnimationFrame can cause leaks but window's doesn't? Sorry, I missed this thread until now (in the future, feel free to kick me over IM if I don't respond in review) In this case, I'm afraid I don't know, I don't know much about requestAnimationFrame or memory issues.
https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:753: void VRDisplay::contextDestroyed() { Why ::contextDestroyed() rather than ::dispose()?
haraken@chromium.org changed reviewers: + haraken@chromium.org, sigbjornf@opera.com
On 2017/01/13 00:34:19, mthiesse wrote: > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:753: void > VRDisplay::contextDestroyed() { > Why ::contextDestroyed() rather than ::dispose()? dispose() is called just before a GC calls a destructor of VRDisplay. In other words, dispose() is not called until VRDisplay becomes unreachable. Sigbjorn: That said, I'm wondering why we need to manually clear m_scriptedAnimationController. It would mean that we have a reference cycle around it but I couldn't find anything...
On 2017/01/13 00:44:23, haraken wrote: > On 2017/01/13 00:34:19, mthiesse wrote: > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:753: void > > VRDisplay::contextDestroyed() { > > Why ::contextDestroyed() rather than ::dispose()? > > dispose() is called just before a GC calls a destructor of VRDisplay. In other > words, dispose() is not called until VRDisplay becomes unreachable. > > Sigbjorn: That said, I'm wondering why we need to manually clear > m_scriptedAnimationController. It would mean that we have a reference cycle > around it but I couldn't find anything... So we want to keep it as contextDestroyed(), not dispose()?
On 2017/01/13 01:47:04, bsheedy wrote: > On 2017/01/13 00:44:23, haraken wrote: > > On 2017/01/13 00:34:19, mthiesse wrote: > > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > > > > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:753: void > > > VRDisplay::contextDestroyed() { > > > Why ::contextDestroyed() rather than ::dispose()? > > > > dispose() is called just before a GC calls a destructor of VRDisplay. In other > > words, dispose() is not called until VRDisplay becomes unreachable. > > > > Sigbjorn: That said, I'm wondering why we need to manually clear > > m_scriptedAnimationController. It would mean that we have a reference cycle > > around it but I couldn't find anything... > > So we want to keep it as contextDestroyed(), not dispose()? Yes, but I'm wondering why we need to clear m_scriptedAnimationController in contextDestroyed in the first place.
On 2017/01/13 02:00:00, haraken wrote: > On 2017/01/13 01:47:04, bsheedy wrote: > > On 2017/01/13 00:44:23, haraken wrote: > > > On 2017/01/13 00:34:19, mthiesse wrote: > > > > > > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:753: void > > > > VRDisplay::contextDestroyed() { > > > > Why ::contextDestroyed() rather than ::dispose()? > > > > > > dispose() is called just before a GC calls a destructor of VRDisplay. In > other > > > words, dispose() is not called until VRDisplay becomes unreachable. > > > > > > Sigbjorn: That said, I'm wondering why we need to manually clear > > > m_scriptedAnimationController. It would mean that we have a reference cycle > > > around it but I couldn't find anything... > > > > So we want to keep it as contextDestroyed(), not dispose()? > > Yes, but I'm wondering why we need to clear m_scriptedAnimationController in > contextDestroyed in the first place. Document::shutdown also explicitly calls ScriptedAnimationController::clear(). Is there a reason for this other than tracing how long shutting down a document takes?
On 2017/01/13 02:14:16, mthiesse wrote: > On 2017/01/13 02:00:00, haraken wrote: > > On 2017/01/13 01:47:04, bsheedy wrote: > > > On 2017/01/13 00:44:23, haraken wrote: > > > > On 2017/01/13 00:34:19, mthiesse wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:753: void > > > > > VRDisplay::contextDestroyed() { > > > > > Why ::contextDestroyed() rather than ::dispose()? > > > > > > > > dispose() is called just before a GC calls a destructor of VRDisplay. In > > other > > > > words, dispose() is not called until VRDisplay becomes unreachable. > > > > > > > > Sigbjorn: That said, I'm wondering why we need to manually clear > > > > m_scriptedAnimationController. It would mean that we have a reference > cycle > > > > around it but I couldn't find anything... > > > > > > So we want to keep it as contextDestroyed(), not dispose()? > > > > Yes, but I'm wondering why we need to clear m_scriptedAnimationController in > > contextDestroyed in the first place. > > Document::shutdown also explicitly calls ScriptedAnimationController::clear(). > Is there a reason for this other than tracing how long shutting down a document > takes? I was thinking that ScriptedAnimationController::clear() is needed for preventing rAF tasks from running after the Document is detached, not for preventing memory leaks. This CL LGTM because VR also needs to prevent rAF tasks from running after the Document is detached. But I still don't understand why it fixes memory leaks.
On 2017/01/13 02:00:00, haraken wrote: > On 2017/01/13 01:47:04, bsheedy wrote: > > On 2017/01/13 00:44:23, haraken wrote: > > > On 2017/01/13 00:34:19, mthiesse wrote: > > > > > > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:753: void > > > > VRDisplay::contextDestroyed() { > > > > Why ::contextDestroyed() rather than ::dispose()? > > > > > > dispose() is called just before a GC calls a destructor of VRDisplay. In > other > > > words, dispose() is not called until VRDisplay becomes unreachable. > > > > > > Sigbjorn: That said, I'm wondering why we need to manually clear > > > m_scriptedAnimationController. It would mean that we have a reference cycle > > > around it but I couldn't find anything... > > > > So we want to keep it as contextDestroyed(), not dispose()? > > Yes, but I'm wondering why we need to clear m_scriptedAnimationController in > contextDestroyed in the first place. I haven't looked in a debugger, but the VRDisplay object is being kept alive by script, judging by the test source. When the document (ExecutionContext) is being destroyed, the Document will (after contextDestroyed()) let go of the reference to the VRDisplay lifecycle observer. So, Document won't keep VRDisplay alive. The wrapper object for VRDisplay will though, so we better not have any reachable reference back to the document to prevent it from being kept alive & leaking. ScriptedAnimationController has a Member<Document>.. Which is why snipping out the connection prevents the leak. An alternative, i believe, is to just call clearDocumentPointer().
On 2017/01/13 06:38:11, sof wrote: > On 2017/01/13 02:00:00, haraken wrote: > > On 2017/01/13 01:47:04, bsheedy wrote: > > > On 2017/01/13 00:44:23, haraken wrote: > > > > On 2017/01/13 00:34:19, mthiesse wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:753: void > > > > > VRDisplay::contextDestroyed() { > > > > > Why ::contextDestroyed() rather than ::dispose()? > > > > > > > > dispose() is called just before a GC calls a destructor of VRDisplay. In > > other > > > > words, dispose() is not called until VRDisplay becomes unreachable. > > > > > > > > Sigbjorn: That said, I'm wondering why we need to manually clear > > > > m_scriptedAnimationController. It would mean that we have a reference > cycle > > > > around it but I couldn't find anything... > > > > > > So we want to keep it as contextDestroyed(), not dispose()? > > > > Yes, but I'm wondering why we need to clear m_scriptedAnimationController in > > contextDestroyed in the first place. > > I haven't looked in a debugger, but the VRDisplay object is being kept alive by > script, judging by the test source. > > When the document (ExecutionContext) is being destroyed, the Document will > (after contextDestroyed()) let go of the reference to the VRDisplay lifecycle > observer. So, Document won't keep VRDisplay alive. The wrapper object for > VRDisplay will though, so we better not have any reachable reference back to the > document to prevent it from being kept alive & leaking. > ScriptedAnimationController has a Member<Document>.. But VRDisplay::hasPendingActivity will return false after the context is detached. Then the wrapper will be collected...? > > Which is why snipping out the connection prevents the leak. An alternative, i > believe, is to just call clearDocumentPointer().
On 2017/01/13 06:38:11, sof wrote: > On 2017/01/13 02:00:00, haraken wrote: > > On 2017/01/13 01:47:04, bsheedy wrote: > > > On 2017/01/13 00:44:23, haraken wrote: > > > > On 2017/01/13 00:34:19, mthiesse wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:753: void > > > > > VRDisplay::contextDestroyed() { > > > > > Why ::contextDestroyed() rather than ::dispose()? > > > > > > > > dispose() is called just before a GC calls a destructor of VRDisplay. In > > other > > > > words, dispose() is not called until VRDisplay becomes unreachable. > > > > > > > > Sigbjorn: That said, I'm wondering why we need to manually clear > > > > m_scriptedAnimationController. It would mean that we have a reference > cycle > > > > around it but I couldn't find anything... > > > > > > So we want to keep it as contextDestroyed(), not dispose()? > > > > Yes, but I'm wondering why we need to clear m_scriptedAnimationController in > > contextDestroyed in the first place. > > I haven't looked in a debugger, but the VRDisplay object is being kept alive by > script, judging by the test source. > > When the document (ExecutionContext) is being destroyed, the Document will > (after contextDestroyed()) let go of the reference to the VRDisplay lifecycle > observer. So, Document won't keep VRDisplay alive. The wrapper object for > VRDisplay will though, so we better not have any reachable reference back to the > document to prevent it from being kept alive & leaking. > ScriptedAnimationController has a Member<Document>.. > > Which is why snipping out the connection prevents the leak. An alternative, i > believe, is to just call clearDocumentPointer(). Hmm, who is retaining VRDisplay in the first place, isn't clear, i now realize. Will look at the details.
On 2017/01/13 06:56:17, sof wrote: > On 2017/01/13 06:38:11, sof wrote: > > On 2017/01/13 02:00:00, haraken wrote: > > > On 2017/01/13 01:47:04, bsheedy wrote: > > > > On 2017/01/13 00:44:23, haraken wrote: > > > > > On 2017/01/13 00:34:19, mthiesse wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > > > > > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:753: void > > > > > > VRDisplay::contextDestroyed() { > > > > > > Why ::contextDestroyed() rather than ::dispose()? > > > > > > > > > > dispose() is called just before a GC calls a destructor of VRDisplay. In > > > other > > > > > words, dispose() is not called until VRDisplay becomes unreachable. > > > > > > > > > > Sigbjorn: That said, I'm wondering why we need to manually clear > > > > > m_scriptedAnimationController. It would mean that we have a reference > > cycle > > > > > around it but I couldn't find anything... > > > > > > > > So we want to keep it as contextDestroyed(), not dispose()? > > > > > > Yes, but I'm wondering why we need to clear m_scriptedAnimationController in > > > contextDestroyed in the first place. > > > > I haven't looked in a debugger, but the VRDisplay object is being kept alive > by > > script, judging by the test source. > > > > When the document (ExecutionContext) is being destroyed, the Document will > > (after contextDestroyed()) let go of the reference to the VRDisplay lifecycle > > observer. So, Document won't keep VRDisplay alive. The wrapper object for > > VRDisplay will though, so we better not have any reachable reference back to > the > > document to prevent it from being kept alive & leaking. > > ScriptedAnimationController has a Member<Document>.. > > > > Which is why snipping out the connection prevents the leak. An alternative, i > > believe, is to just call clearDocumentPointer(). > > Hmm, who is retaining VRDisplay in the first place, isn't clear, i now realize. > Will look at the details. It is the script holding onto VRDisplay, and issuing requestAnimationFrame() calls after the test framework's done() has been executed. Unclear if those rAF calls are intentionally done, but we shouldn't leak in any case. (hasPendingActivity() will return |false| post-contextDestroyed() as expected, so VRDisplay isn't keeping itself alive.)
https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/vr/requestAnimationFrame_invalidhandle.html (right): https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vr/requestAnimationFrame_invalidhandle.html:19: t.done(); this looks like it is pumping onAnimationFrame() calls even after done() has been called, is that what you're after? https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/vr/requestAnimationFrame_unregister.html (right): https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vr/requestAnimationFrame_unregister.html:25: t.done(); is it intentional not to "return" here after signalling done()?
non-owner lgtm. Unless rAF can be extended to throw on detached document use, it has to support these without fail, so if you could add comments to the tests to indicate that this is what's (also) going on, that'd be worth it. The description also needs to be synced.
Looking at the VRDisplay's implementation of rAF, it will return 0 when detached - is it allowed to do that per spec?
On 2017/01/13 08:41:24, sof wrote: > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/vr/requestAnimationFrame_invalidhandle.html > (right): > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/vr/requestAnimationFrame_invalidhandle.html:19: > t.done(); > this looks like it is pumping onAnimationFrame() calls even after done() has > been called, is that what you're after? > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/vr/requestAnimationFrame_unregister.html:25: > t.done(); > is it intentional not to "return" here after signalling done()? It wasn't originally intentional to not return after done() and continue to call rAF. However, since the leak appears to be fixed if we don't have any outstanding rAF callbacks when the test ends (make sure rAF is called somewhere further down than t.done() and return immediately after t.done()), I intentionally left it as-is. We shouldn't get any leaks regardless of what Javascript code is being run, so keeping it this way adds a little bit of extra test coverage to make sure we aren't leaking even if we're doing weird things in Javascript. On 2017/01/13 08:58:08, sof wrote: > Looking at the VRDisplay's implementation of rAF, it will return 0 when detached > - is it allowed to do that per spec? The spec only requires that rAF return an unsigned long, so it's allowed AFAIK.
Description was changed from ========== Fix WebVR requestAnimationFrame leaking VRDisplay's requestAnimationFrame now keeps track of any pending callbacks and cancels them when the display's context is destroyed. This prevents leaking that was occuring if a test finished while there were still pending callbacks. Re-add the two layout tests that were failing due to detecting the leaks. BUG=679401 ========== to ========== Fix WebVR requestAnimationFrame leaking Manually clear the Member<ScriptedAnimationController> handle in VRDisplay like Document does in order to prevent leaks when the Document is destroyed and there are still outstanding requestAnimationFrame callbacks. Re-add the two layout tests that were failing due to detecting the leaks. BUG=679401 ==========
Added comments to tests as suggested by sof@. It seems like people agree this is ready to submit, so unless there are any objections in the next hour or so, I'll submit. https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/vr/requestAnimationFrame_invalidhandle.html (right): https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vr/requestAnimationFrame_invalidhandle.html:19: t.done(); On 2017/01/13 08:41:24, sof wrote: > this looks like it is pumping onAnimationFrame() calls even after done() has > been called, is that what you're after? See earlier reply to your message. https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/vr/requestAnimationFrame_unregister.html (right): https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vr/requestAnimationFrame_unregister.html:25: t.done(); On 2017/01/13 08:41:24, sof wrote: > is it intentional not to "return" here after signalling done()? Yes, see earlier reply to your message.
On 2017/01/13 17:40:30, bsheedy wrote: > On 2017/01/13 08:41:24, sof wrote: > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Layo... > > File > third_party/WebKit/LayoutTests/vr/requestAnimationFrame_invalidhandle.html > > (right): > > > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/vr/requestAnimationFrame_invalidhandle.html:19: > > t.done(); > > this looks like it is pumping onAnimationFrame() calls even after done() has > > been called, is that what you're after? > > > https://codereview.chromium.org/2622943002/diff/40001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/vr/requestAnimationFrame_unregister.html:25: > > t.done(); > > is it intentional not to "return" here after signalling done()? > It wasn't originally intentional to not return after done() and continue to call > rAF. However, since the leak appears to be fixed if we don't have any > outstanding rAF callbacks when the test ends (make sure rAF is called somewhere > further down than t.done() and return immediately after t.done()), I > intentionally left it as-is. We shouldn't get any leaks regardless of what > Javascript code is being run, so keeping it this way adds a little bit of extra > test coverage to make sure we aren't leaking even if we're doing weird things in > Javascript. > > On 2017/01/13 08:58:08, sof wrote: > > Looking at the VRDisplay's implementation of rAF, it will return 0 when > detached > > - is it allowed to do that per spec? > The spec only requires that rAF return an unsigned long, so it's allowed AFAIK. An edge case this, but the spec equates it as being like window.requestAnimationFrame(), which requires a unique value to be returned.
The CQ bit was checked by bsheedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, sigbjornf@opera.com, bajones@chromium.org Link to the patchset: https://codereview.chromium.org/2622943002/#ps60001 (title: "Added comments to tests explaining why they end with outstanding callbacks")
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bsheedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, sigbjornf@opera.com, bajones@chromium.org Link to the patchset: https://codereview.chromium.org/2622943002/#ps80001 (title: "Rebase to see if CQ failure fixed")
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": 80001, "attempt_start_ts": 1484343107532400,
"parent_rev": "f4747e7d46e184744730cf6096c12d775262c9e2", "commit_rev":
"d8028087727891a6575e8209daecf58035a95723"}
Message was sent while issue was closed.
Description was changed from ========== Fix WebVR requestAnimationFrame leaking Manually clear the Member<ScriptedAnimationController> handle in VRDisplay like Document does in order to prevent leaks when the Document is destroyed and there are still outstanding requestAnimationFrame callbacks. Re-add the two layout tests that were failing due to detecting the leaks. BUG=679401 ========== to ========== Fix WebVR requestAnimationFrame leaking Manually clear the Member<ScriptedAnimationController> handle in VRDisplay like Document does in order to prevent leaks when the Document is destroyed and there are still outstanding requestAnimationFrame callbacks. Re-add the two layout tests that were failing due to detecting the leaks. BUG=679401 Review-Url: https://codereview.chromium.org/2622943002 Cr-Commit-Position: refs/heads/master@{#443711} Committed: https://chromium.googlesource.com/chromium/src/+/d8028087727891a6575e8209daec... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/d8028087727891a6575e8209daec... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
