Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2622193002:
Dispatch slotchange events in "notify mutation observers" steps (Closed)
DescriptionDispatch slotchange events in "notify mutation observers" steps
The spec is: https://dom.spec.whatwg.org/#notify-mutation-observers
The current behavior does not match the DOM Standard. The difference is observable
if a user mutates DOM, which will trigger a slotchange, in a mutation observer's
callback function.
See crbug.com/671976 for details.
This CL fixes a failing wpt test, wpt/shadow-dom/slotchnage-event.html.
BUG=649159, 671976
Review-Url: https://codereview.chromium.org/2622193002
Cr-Commit-Position: refs/heads/master@{#443866}
Committed: https://chromium.googlesource.com/chromium/src/+/c806e7eaec2921239800ad1a907b3d347f4f4a30
Patch Set 1 #
Total comments: 5
Patch Set 2 : fixed #
Total comments: 2
Patch Set 3 : clean slotchange list #
Total comments: 2
Patch Set 4 : fixed #
Messages
Total messages: 46 (22 generated)
The CQ bit was checked by hayato@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 ========== Fix the timing of dispatching slotchange events BUG=49159,671976 ========== to ========== Dispatch slotchange events in "notify mutation observers" steps The spec is: https://dom.spec.whatwg.org/#notify-mutation-observers See https://bugs.chromium.org/p/chromium/issues/detail?id=671976 for details. The current behavior does not match the DOM Standard. The difference is observable if users mutates DOM in a mutation observer. This CL fixes a failing wpt test, shadow-dom/slotchnage-event.html. BUG=49159,671976 ==========
Description was changed from ========== Dispatch slotchange events in "notify mutation observers" steps The spec is: https://dom.spec.whatwg.org/#notify-mutation-observers See https://bugs.chromium.org/p/chromium/issues/detail?id=671976 for details. The current behavior does not match the DOM Standard. The difference is observable if users mutates DOM in a mutation observer. This CL fixes a failing wpt test, shadow-dom/slotchnage-event.html. BUG=49159,671976 ========== to ========== Dispatch slotchange events in "notify mutation observers" steps The spec is: https://dom.spec.whatwg.org/#notify-mutation-observers See https://bugs.chromium.org/p/chromium/issues/detail?id=671976 for details. The current behavior does not match the DOM Standard. The difference is observable if a user mutates DOM, which will trigger a slotchange, in a mutation observer's callback function. This CL fixes a failing wpt test, shadow-dom/slotchnage-event.html. BUG=49159,671976 ==========
hayato@chromium.org changed reviewers: + kochi@chromium.org, tkent@chromium.org
PTAL
Description was changed from ========== Dispatch slotchange events in "notify mutation observers" steps The spec is: https://dom.spec.whatwg.org/#notify-mutation-observers See https://bugs.chromium.org/p/chromium/issues/detail?id=671976 for details. The current behavior does not match the DOM Standard. The difference is observable if a user mutates DOM, which will trigger a slotchange, in a mutation observer's callback function. This CL fixes a failing wpt test, shadow-dom/slotchnage-event.html. BUG=49159,671976 ========== to ========== Dispatch slotchange events in "notify mutation observers" steps The spec is: https://dom.spec.whatwg.org/#notify-mutation-observers See https://bugs.chromium.org/p/chromium/issues/detail?id=671976 for details. The current behavior does not match the DOM Standard. The difference is observable if a user mutates DOM, which will trigger a slotchange, in a mutation observer's callback function. This CL fixes a failing wpt test, wpt/shadow-dom/slotchnage-event.html. BUG=49159,671976 ==========
Description was changed from ========== Dispatch slotchange events in "notify mutation observers" steps The spec is: https://dom.spec.whatwg.org/#notify-mutation-observers See https://bugs.chromium.org/p/chromium/issues/detail?id=671976 for details. The current behavior does not match the DOM Standard. The difference is observable if a user mutates DOM, which will trigger a slotchange, in a mutation observer's callback function. This CL fixes a failing wpt test, wpt/shadow-dom/slotchnage-event.html. BUG=49159,671976 ========== to ========== Dispatch slotchange events in "notify mutation observers" steps The spec is: https://dom.spec.whatwg.org/#notify-mutation-observers The current behavior does not match the DOM Standard. The difference is observable if a user mutates DOM, which will trigger a slotchange, in a mutation observer's callback function. See crbug.com/671976 for details. This CL fixes a failing wpt test, wpt/shadow-dom/slotchnage-event.html. BUG=49159,671976 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Dispatch slotchange events in "notify mutation observers" steps The spec is: https://dom.spec.whatwg.org/#notify-mutation-observers The current behavior does not match the DOM Standard. The difference is observable if a user mutates DOM, which will trigger a slotchange, in a mutation observer's callback function. See crbug.com/671976 for details. This CL fixes a failing wpt test, wpt/shadow-dom/slotchnage-event.html. BUG=49159,671976 ========== to ========== Dispatch slotchange events in "notify mutation observers" steps The spec is: https://dom.spec.whatwg.org/#notify-mutation-observers The current behavior does not match the DOM Standard. The difference is observable if a user mutates DOM, which will trigger a slotchange, in a mutation observer's callback function. See crbug.com/671976 for details. This CL fixes a failing wpt test, wpt/shadow-dom/slotchnage-event.html. BUG=649159,671976 ==========
https://codereview.chromium.org/2622193002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/MutationObserver.cpp (right): https://codereview.chromium.org/2622193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/MutationObserver.cpp:181: DEFINE_STATIC_LOCAL(SlotChangeList, slotChangeList, (new SlotChangeList)); I'm afraid static strong references to HTMLSlotElements can cause memory leak. Is this safe? https://codereview.chromium.org/2622193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/MutationObserver.cpp:289: copyToVector(activeSlotChangeList(), slots); slots.swap(activeSlotChangeList()) should work.
fixed
The CQ bit was checked by hayato@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: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Thank you for the review. https://codereview.chromium.org/2622193002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/MutationObserver.cpp (right): https://codereview.chromium.org/2622193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/MutationObserver.cpp:181: DEFINE_STATIC_LOCAL(SlotChangeList, slotChangeList, (new SlotChangeList)); On 2017/01/11 at 11:24:17, tkent wrote: > I'm afraid static strong references to HTMLSlotElements can cause memory leak. Is this safe? The content of |slotlist| is always cleared at the timing of microtask. Thus, I do not think memory leak would happen. Rather, I had a concern about using HeapVector in a function static variable. Does Oilpan trace it? It looks existing code, e.g. activeMutationObservers(), uses HeapHashSet<Member<MutationObserver>>, in a function static local. Thus, I thought it is okay. https://codereview.chromium.org/2622193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/MutationObserver.cpp:289: copyToVector(activeSlotChangeList(), slots); Done
lgtm https://codereview.chromium.org/2622193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/MutationObserver.cpp (right): https://codereview.chromium.org/2622193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/MutationObserver.cpp:286: activeMutationObservers().clear(); Can the same .swap() be used for activeMutationObservers()? (don't have to be in this CL)
https://codereview.chromium.org/2622193002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/MutationObserver.cpp (right): https://codereview.chromium.org/2622193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/MutationObserver.cpp:181: DEFINE_STATIC_LOCAL(SlotChangeList, slotChangeList, (new SlotChangeList)); On 2017/01/12 at 03:47:41, hayato wrote: > On 2017/01/11 at 11:24:17, tkent wrote: > > I'm afraid static strong references to HTMLSlotElements can cause memory leak. Is this safe? > > The content of |slotlist| is always cleared at the timing of microtask. Thus, I do not think memory leak would happen. Is the microtask guaranteed to be executed? Is v8::Isolate::TearDown() never called when its microtask queue is not empty? > Rather, I had a concern about using HeapVector in a function static variable. Does Oilpan trace it? > It looks existing code, e.g. activeMutationObservers(), uses HeapHashSet<Member<MutationObserver>>, in a function static local. Thus, I thought it is okay. It's ok. DEFINE_STATIC_LOCAL uses Persistent<T> internally for Oilpan objects.
https://codereview.chromium.org/2622193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/MutationObserver.cpp (right): https://codereview.chromium.org/2622193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/MutationObserver.cpp:286: activeMutationObservers().clear(); On 2017/01/12 at 04:57:55, kochi wrote: > Can the same .swap() be used for activeMutationObservers()? > > (don't have to be in this CL) No. activeMutationObserevers() returns HeapHashSet, instead of HeapVector. We can not swap them.
The CQ bit was checked by hayato@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/12 06:30:17, hayato wrote: > https://codereview.chromium.org/2622193002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/MutationObserver.cpp (right): > > https://codereview.chromium.org/2622193002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/MutationObserver.cpp:286: > activeMutationObservers().clear(); > On 2017/01/12 at 04:57:55, kochi wrote: > > Can the same .swap() be used for activeMutationObservers()? > > > > (don't have to be in this CL) > > No. activeMutationObserevers() returns HeapHashSet, instead of HeapVector. We > can not swap them. I see. Is tkent's concern (microtask is guaranteed to run?) addressed?
On 2017/01/12 at 07:18:05, kochi wrote: > On 2017/01/12 06:30:17, hayato wrote: > > https://codereview.chromium.org/2622193002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/dom/MutationObserver.cpp (right): > > > > https://codereview.chromium.org/2622193002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/dom/MutationObserver.cpp:286: > > activeMutationObservers().clear(); > > On 2017/01/12 at 04:57:55, kochi wrote: > > > Can the same .swap() be used for activeMutationObservers()? > > > > > > (don't have to be in this CL) > > > > No. activeMutationObserevers() returns HeapHashSet, instead of HeapVector. We > > can not swap them. > > I see. > > Is tkent's concern (microtask is guaranteed to run?) addressed? Microtask is enqueued when the vector becomes non-empty. That is what I intended to.
On 2017/01/12 at 07:28:51, hayato wrote: > On 2017/01/12 at 07:18:05, kochi wrote: > > On 2017/01/12 06:30:17, hayato wrote: > > > https://codereview.chromium.org/2622193002/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/dom/MutationObserver.cpp (right): > > > > > > https://codereview.chromium.org/2622193002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/dom/MutationObserver.cpp:286: > > > activeMutationObservers().clear(); > > > On 2017/01/12 at 04:57:55, kochi wrote: > > > > Can the same .swap() be used for activeMutationObservers()? > > > > > > > > (don't have to be in this CL) > > > > > > No. activeMutationObserevers() returns HeapHashSet, instead of HeapVector. We > > > can not swap them. > > > > I see. > > > > Is tkent's concern (microtask is guaranteed to run?) addressed? > > Microtask is enqueued when the vector becomes non-empty. That is what I intended to. I mentioned microtask queue in v8::Isolate, not a vector of HTMLSlotElement.
The CQ bit was unchecked by tkent@chromium.org
On 2017/01/12 at 07:47:47, tkent wrote: > On 2017/01/12 at 07:28:51, hayato wrote: > > On 2017/01/12 at 07:18:05, kochi wrote: > > > On 2017/01/12 06:30:17, hayato wrote: > > > > https://codereview.chromium.org/2622193002/diff/20001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/dom/MutationObserver.cpp (right): > > > > > > > > https://codereview.chromium.org/2622193002/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/dom/MutationObserver.cpp:286: > > > > activeMutationObservers().clear(); > > > > On 2017/01/12 at 04:57:55, kochi wrote: > > > > > Can the same .swap() be used for activeMutationObservers()? > > > > > > > > > > (don't have to be in this CL) > > > > > > > > No. activeMutationObserevers() returns HeapHashSet, instead of HeapVector. We > > > > can not swap them. > > > > > > I see. > > > > > > Is tkent's concern (microtask is guaranteed to run?) addressed? > > > > Microtask is enqueued when the vector becomes non-empty. That is what I intended to. > > I mentioned microtask queue in v8::Isolate, not a vector of HTMLSlotElement. Oh. I have not read comment #20. Let me check.
On 2017/01/12 at 06:24:36, tkent wrote: > https://codereview.chromium.org/2622193002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/dom/MutationObserver.cpp (right): > > https://codereview.chromium.org/2622193002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/dom/MutationObserver.cpp:181: DEFINE_STATIC_LOCAL(SlotChangeList, slotChangeList, (new SlotChangeList)); > On 2017/01/12 at 03:47:41, hayato wrote: > > On 2017/01/11 at 11:24:17, tkent wrote: > > > I'm afraid static strong references to HTMLSlotElements can cause memory leak. Is this safe? > > > > The content of |slotlist| is always cleared at the timing of microtask. Thus, I do not think memory leak would happen. > > Is the microtask guaranteed to be executed? Is v8::Isolate::TearDown() never called when its microtask queue is not empty? I never thought such a case; microtask is never executed. Could you help me to understand when that happens? Does that mean the current MutationObserver's implementation has the same problem? My assumption is that it is always executed.
I do not have a good theory when a queueud microtasks is not executed. To prevent a possible memory leak, what do you think about using a static weak pointer to a slotlist, which is now being owned by microtask itself, instead of storing it in static. Is this worth trying?
On 2017/01/13 at 07:43:10, hayato wrote: > I do not have a good theory when a queueud microtasks is not executed. > > To prevent a possible memory leak, what do you think about using a static weak pointer to a slotlist, which is now being owned by microtask itself, instead of storing it in static. > Is this worth trying? WeakPersistent<HeapVector<Member<HTMLSlotElement>> won't help. It's garbage-collected whenever GC happens. HeapVector<WeakMember<HTMLSlotElement>> is better. However, the HeapVector might leak though HTMLSlotElement won't leak. IMO, storing the vector in Document or an object of which lifetime is same as Document is a solution.
On 2017/01/13 at 08:31:04, tkent wrote: > On 2017/01/13 at 07:43:10, hayato wrote: > > I do not have a good theory when a queueud microtasks is not executed. > > > > To prevent a possible memory leak, what do you think about using a static weak pointer to a slotlist, which is now being owned by microtask itself, instead of storing it in static. > > Is this worth trying? > > WeakPersistent<HeapVector<Member<HTMLSlotElement>> won't help. It's garbage-collected whenever GC happens. > HeapVector<WeakMember<HTMLSlotElement>> is better. However, the HeapVector might leak though HTMLSlotElement won't leak. > > IMO, storing the vector in Document or an object of which lifetime is same as Document is a solution. I see. Thank you. Let me use Document. I still have a concern that a event loop and a Document is not 1:1 relationship. However, this would not be a strong issue in practice, I think.
clean slotchange list
The CQ bit was checked by hayato@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...
I have updated the CL. PTAL. - As we chatted offline, this CL uses static slotchange list, as the previous CL does. This has not changed. - To prevent a possible leak, now Document::shutdown() calls MutationObserver::cleanSlotChangeList(Document&), where unnecessary slots are removed from the list.
lgtm
lgtm https://codereview.chromium.org/2622193002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2622193002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.h:1432: void clearSlotChangeList(); Forgot to remove this line?
fixed
The CQ bit was checked by hayato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, kochi@chromium.org Link to the patchset: https://codereview.chromium.org/2622193002/#ps60001 (title: "fixed")
https://codereview.chromium.org/2622193002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2622193002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.h:1432: void clearSlotChangeList(); On 2017/01/16 at 07:07:42, kochi wrote: > Forgot to remove this line? Nice catch! Let me remove this before landing this CL.
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": 60001, "attempt_start_ts": 1484554264349630,
"parent_rev": "980d3ef4c00833fdccfdfe4475518c1777e9f667", "commit_rev":
"c806e7eaec2921239800ad1a907b3d347f4f4a30"}
Message was sent while issue was closed.
Description was changed from ========== Dispatch slotchange events in "notify mutation observers" steps The spec is: https://dom.spec.whatwg.org/#notify-mutation-observers The current behavior does not match the DOM Standard. The difference is observable if a user mutates DOM, which will trigger a slotchange, in a mutation observer's callback function. See crbug.com/671976 for details. This CL fixes a failing wpt test, wpt/shadow-dom/slotchnage-event.html. BUG=649159,671976 ========== to ========== Dispatch slotchange events in "notify mutation observers" steps The spec is: https://dom.spec.whatwg.org/#notify-mutation-observers The current behavior does not match the DOM Standard. The difference is observable if a user mutates DOM, which will trigger a slotchange, in a mutation observer's callback function. See crbug.com/671976 for details. This CL fixes a failing wpt test, wpt/shadow-dom/slotchnage-event.html. BUG=649159,671976 Review-Url: https://codereview.chromium.org/2622193002 Cr-Commit-Position: refs/heads/master@{#443866} Committed: https://chromium.googlesource.com/chromium/src/+/c806e7eaec2921239800ad1a907b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c806e7eaec2921239800ad1a907b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
