Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2560473005:
[wrapper-tracing] WebGLRenderingContextBase: Fix wrapper tracing (Closed)
|
Created:
4 years ago by Michael Lippautz Modified:
4 years ago CC:
chromium-reviews, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[wrapper-tracing] WebGLRenderingContextBase: Fix wrapper tracing
BUG=671942
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/7737c8e7d1f6cbf4fdae1fd319709b311254d0e3
Cr-Commit-Position: refs/heads/master@{#436919}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 22 (11 generated)
Description was changed from ========== [wrapper-tracing] WebGLRenderingContextBase: Fix wrapper tracing BUG= ========== to ========== [wrapper-tracing] WebGLRenderingContextBase: Fix wrapper tracing BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== [wrapper-tracing] WebGLRenderingContextBase: Fix wrapper tracing BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [wrapper-tracing] WebGLRenderingContextBase: Fix wrapper tracing BUG=671942 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
mlippautz@chromium.org changed reviewers: + haraken@chromium.org, hlopko@chromium.org, kbr@chromium.org
The bug indicates that we miss a write barrier *or* don't trace an object. Presumably (see comments), the guard is wrong here. https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (left): https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:7679: if (isContextLost()) { I need some help with proper semantics here: It is now possible to revive a rendering context, right? E.g. the state without a context can only be transient. If so, then we should trace wrappers no matter what. (This was copied from regular Oilpan tracing but it has been removed there.) https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:7679: visitor->traceWrappers(m_contextGroup); The context group is TraceWrapperBase which indicates we should trace wrappers through its contexts. However, I don't see an IDL file and m_contextGroup is also not part of WebGLRenderingContextBase::visitChildDOMWrappers, so I am not sure what the expected behavior is.
https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:7679: visitor->traceWrappers(m_contextGroup); On 2016/12/07 08:36:11, Michael Lippautz wrote: > The context group is TraceWrapperBase which indicates we should trace wrappers > through its contexts. > > However, I don't see an IDL file and m_contextGroup is also not part of > WebGLRenderingContextBase::visitChildDOMWrappers, so I am not sure what the > expected behavior is. kbr@: Would you help me understand why we need to trace m_contextGroup? This is doing something strange because WebGLContextGroup::m_contexts is a hash map of weak members. We're tracing the members weakly in oilpan but strongly in V8. (We're doing the strange thing in Document::m_nodeLists but that's a very special case.)
LGTM++ with whatever fix will make the test stop crashing. https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:7679: visitor->traceWrappers(m_contextGroup); On 2016/12/07 09:06:29, haraken wrote: > On 2016/12/07 08:36:11, Michael Lippautz wrote: > > The context group is TraceWrapperBase which indicates we should trace wrappers > > through its contexts. > > > > However, I don't see an IDL file and m_contextGroup is also not part of > > WebGLRenderingContextBase::visitChildDOMWrappers, so I am not sure what the > > expected behavior is. > > kbr@: Would you help me understand why we need to trace m_contextGroup? This is > doing something strange because WebGLContextGroup::m_contexts is a hash map of > weak members. We're tracing the members weakly in oilpan but strongly in V8. > > (We're doing the strange thing in Document::m_nodeLists but that's a very > special case.) > WebGLRenderingContextBase strongly owns WebGLContextGroup. WebGLContextGroup maintains weak references back to the WebGLContextBases. (Right now there is only a 1:1 mapping. We will probably get rid of WebGLContextGroup, at which point the lifetimes will be a lot simpler) WebGLObjects maintain strong references to *either* their allocating WebGLRenderingContextBase, or WebGLContextGroup, depending on whether they conceptually are per-context or per-share-group objects. WebGLContextGroup isn't a concept publicly visible to JS. However, we must trace from WebGLSharedObject to its WebGLContextGroup, and from there to any WebGLRenderingContextBase objects in that WebGLContextGroup.
On 2016/12/07 09:22:37, Ken Russell wrote: > LGTM++ with whatever fix will make the test stop crashing. > > https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp > (right): > > https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:7679: > visitor->traceWrappers(m_contextGroup); > On 2016/12/07 09:06:29, haraken wrote: > > On 2016/12/07 08:36:11, Michael Lippautz wrote: > > > The context group is TraceWrapperBase which indicates we should trace > wrappers > > > through its contexts. > > > > > > However, I don't see an IDL file and m_contextGroup is also not part of > > > WebGLRenderingContextBase::visitChildDOMWrappers, so I am not sure what the > > > expected behavior is. > > > > kbr@: Would you help me understand why we need to trace m_contextGroup? This > is > > doing something strange because WebGLContextGroup::m_contexts is a hash map > of > > weak members. We're tracing the members weakly in oilpan but strongly in V8. > > > > (We're doing the strange thing in Document::m_nodeLists but that's a very > > special case.) > > > > WebGLRenderingContextBase strongly owns WebGLContextGroup. WebGLContextGroup > maintains weak references back to the WebGLContextBases. (Right now there is > only a 1:1 mapping. We will probably get rid of WebGLContextGroup, at which > point the lifetimes will be a lot simpler) Why do the back references need to be weak? If the mapping is 1:1, can they be strong members? > > WebGLObjects maintain strong references to *either* their allocating > WebGLRenderingContextBase, or WebGLContextGroup, depending on whether they > conceptually are per-context or per-share-group objects. > > WebGLContextGroup isn't a concept publicly visible to JS. However, we must trace > from WebGLSharedObject to its WebGLContextGroup, and from there to any > WebGLRenderingContextBase objects in that WebGLContextGroup. It's not common that a pointer is traced weakly in oilpan but strongly in V8. Would you add a comment after Michael lands this CL?
The CQ bit was checked by mlippautz@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.
On 2016/12/07 09:26:27, haraken wrote: > On 2016/12/07 09:22:37, Ken Russell wrote: > > LGTM++ with whatever fix will make the test stop crashing. > > > > > https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp > > (right): > > > > > https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:7679: > > visitor->traceWrappers(m_contextGroup); > > On 2016/12/07 09:06:29, haraken wrote: > > > On 2016/12/07 08:36:11, Michael Lippautz wrote: > > > > The context group is TraceWrapperBase which indicates we should trace > > wrappers > > > > through its contexts. > > > > > > > > However, I don't see an IDL file and m_contextGroup is also not part of > > > > WebGLRenderingContextBase::visitChildDOMWrappers, so I am not sure what > the > > > > expected behavior is. > > > > > > kbr@: Would you help me understand why we need to trace m_contextGroup? This > > is > > > doing something strange because WebGLContextGroup::m_contexts is a hash map > > of > > > weak members. We're tracing the members weakly in oilpan but strongly in V8. > > > > > > (We're doing the strange thing in Document::m_nodeLists but that's a very > > > special case.) > > > > > > > WebGLRenderingContextBase strongly owns WebGLContextGroup. WebGLContextGroup > > maintains weak references back to the WebGLContextBases. (Right now there is > > only a 1:1 mapping. We will probably get rid of WebGLContextGroup, at which > > point the lifetimes will be a lot simpler) > > Why do the back references need to be weak? If the mapping is 1:1, can they be > strong members? > > > > > WebGLObjects maintain strong references to *either* their allocating > > WebGLRenderingContextBase, or WebGLContextGroup, depending on whether they > > conceptually are per-context or per-share-group objects. > > > > WebGLContextGroup isn't a concept publicly visible to JS. However, we must > trace > > from WebGLSharedObject to its WebGLContextGroup, and from there to any > > WebGLRenderingContextBase objects in that WebGLContextGroup. > > It's not common that a pointer is traced weakly in oilpan but strongly in V8. > Would you add a comment after Michael lands this CL? Will commit this now to check whether it helps the bots.
The CQ bit was checked by mlippautz@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": 1481111405902040, "parent_rev":
"1d82f096572abd7c210c4bd46a54067d4681f33a", "commit_rev":
"ed0c22a7160d357fb92c7785fede18350e32c02d"}
Message was sent while issue was closed.
Description was changed from ========== [wrapper-tracing] WebGLRenderingContextBase: Fix wrapper tracing BUG=671942 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [wrapper-tracing] WebGLRenderingContextBase: Fix wrapper tracing BUG=671942 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [wrapper-tracing] WebGLRenderingContextBase: Fix wrapper tracing BUG=671942 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [wrapper-tracing] WebGLRenderingContextBase: Fix wrapper tracing BUG=671942 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/7737c8e7d1f6cbf4fdae1fd319709b311254d0e3 Cr-Commit-Position: refs/heads/master@{#436919} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/7737c8e7d1f6cbf4fdae1fd319709b311254d0e3 Cr-Commit-Position: refs/heads/master@{#436919}
Message was sent while issue was closed.
This CL LGTM. kbr@: I have a question on #7.
Message was sent while issue was closed.
On 2016/12/07 09:26:27, haraken wrote: > On 2016/12/07 09:22:37, Ken Russell wrote: > > LGTM++ with whatever fix will make the test stop crashing. > > > > > https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp > > (right): > > > > > https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:7679: > > visitor->traceWrappers(m_contextGroup); > > On 2016/12/07 09:06:29, haraken wrote: > > > On 2016/12/07 08:36:11, Michael Lippautz wrote: > > > > The context group is TraceWrapperBase which indicates we should trace > > wrappers > > > > through its contexts. > > > > > > > > However, I don't see an IDL file and m_contextGroup is also not part of > > > > WebGLRenderingContextBase::visitChildDOMWrappers, so I am not sure what > the > > > > expected behavior is. > > > > > > kbr@: Would you help me understand why we need to trace m_contextGroup? This > > is > > > doing something strange because WebGLContextGroup::m_contexts is a hash map > > of > > > weak members. We're tracing the members weakly in oilpan but strongly in V8. > > > > > > (We're doing the strange thing in Document::m_nodeLists but that's a very > > > special case.) > > > > > > > WebGLRenderingContextBase strongly owns WebGLContextGroup. WebGLContextGroup > > maintains weak references back to the WebGLContextBases. (Right now there is > > only a 1:1 mapping. We will probably get rid of WebGLContextGroup, at which > > point the lifetimes will be a lot simpler) > > Why do the back references need to be weak? If the mapping is 1:1, can they be > strong members? (Appreciate it if you could use the inline code review comments. It's more difficult to follow the conversation otherwise) It's not mandatory that the back references be weak, but that better models the lifetimes of the OpenGL contexts according to my understanding of that API. We could change them to strong references. Would that change the behavior in any significant way? > > WebGLObjects maintain strong references to *either* their allocating > > WebGLRenderingContextBase, or WebGLContextGroup, depending on whether they > > conceptually are per-context or per-share-group objects. > > > > WebGLContextGroup isn't a concept publicly visible to JS. However, we must > trace > > from WebGLSharedObject to its WebGLContextGroup, and from there to any > > WebGLRenderingContextBase objects in that WebGLContextGroup. > > It's not common that a pointer is traced weakly in oilpan but strongly in V8. > Would you add a comment after Michael lands this CL? Let's continue to work on that rather than simply add a comment.
Message was sent while issue was closed.
On 2016/12/08 01:42:17, Ken Russell wrote: > On 2016/12/07 09:26:27, haraken wrote: > > On 2016/12/07 09:22:37, Ken Russell wrote: > > > LGTM++ with whatever fix will make the test stop crashing. > > > > > > > > > https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... > > > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2560473005/diff/1/third_party/WebKit/Source/m... > > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:7679: > > > visitor->traceWrappers(m_contextGroup); > > > On 2016/12/07 09:06:29, haraken wrote: > > > > On 2016/12/07 08:36:11, Michael Lippautz wrote: > > > > > The context group is TraceWrapperBase which indicates we should trace > > > wrappers > > > > > through its contexts. > > > > > > > > > > However, I don't see an IDL file and m_contextGroup is also not part of > > > > > WebGLRenderingContextBase::visitChildDOMWrappers, so I am not sure what > > the > > > > > expected behavior is. > > > > > > > > kbr@: Would you help me understand why we need to trace m_contextGroup? > This > > > is > > > > doing something strange because WebGLContextGroup::m_contexts is a hash > map > > > of > > > > weak members. We're tracing the members weakly in oilpan but strongly in > V8. > > > > > > > > (We're doing the strange thing in Document::m_nodeLists but that's a very > > > > special case.) > > > > > > > > > > WebGLRenderingContextBase strongly owns WebGLContextGroup. WebGLContextGroup > > > maintains weak references back to the WebGLContextBases. (Right now there is > > > only a 1:1 mapping. We will probably get rid of WebGLContextGroup, at which > > > point the lifetimes will be a lot simpler) > > > > Why do the back references need to be weak? If the mapping is 1:1, can they be > > strong members? > > (Appreciate it if you could use the inline code review comments. It's more > difficult to follow the conversation otherwise) > > It's not mandatory that the back references be weak, but that better models the > lifetimes of the OpenGL contexts according to my understanding of that API. > > We could change them to strong references. Would that change the behavior in any > significant way? I don't think so, and that's why I prefer just using strong members. A weak member should be used only when an object X may outlives an object Y (to represent a reference from X to Y). Otherwise, we should use a strong member. It's very strange that you're tracing the hash set weakly in oilpan but strongly in V8. If you use a strong member, you can just use TraceWrapperMember and won't need the manual write barrier. > > > > WebGLObjects maintain strong references to *either* their allocating > > > WebGLRenderingContextBase, or WebGLContextGroup, depending on whether they > > > conceptually are per-context or per-share-group objects. > > > > > > WebGLContextGroup isn't a concept publicly visible to JS. However, we must > > trace > > > from WebGLSharedObject to its WebGLContextGroup, and from there to any > > > WebGLRenderingContextBase objects in that WebGLContextGroup. > > > > It's not common that a pointer is traced weakly in oilpan but strongly in V8. > > Would you add a comment after Michael lands this CL? > > Let's continue to work on that rather than simply add a comment. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
