Summary: | http/tests/workers/service/ServiceWorkerGlobalScope-properties.html is flaky | ||
---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> |
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> |
Status: | RESOLVED DUPLICATE | ||
Severity: | Normal | CC: | beidson, ggaren, jlewis3, youennf |
Priority: | P2 | ||
Version: | WebKit Nightly Build | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=179067 |
Description
Chris Dumez
2017-10-31 10:40:36 PDT
The timeouts / crashes seem to go away with the fix for Bug 179067. I'll investigate why. (In reply to Chris Dumez from comment #1) > The timeouts / crashes seem to go away with the fix for Bug 179067. I'll > investigate why. The assertion that was hit in JSEventListener::jsFunction() is: // Verify that we have a valid wrapper protecting our function from // garbage collection. That is except for when we're not in the normal // world and can have zombie m_jsFunctions. ASSERT(!m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction); So this seems to indicate that the JSEventListener object's wrapper has been collected too early. Normally, such collection is prevented by JSEventTarget's visitor: void JSEventTarget::visitAdditionalChildren(SlotVisitor& visitor) { wrapped().visitJSEventListeners(visitor); } JSServiceWorkerContainer subclasses JSEventTargets and should therefore use this visiting code. (In reply to Chris Dumez from comment #2) > (In reply to Chris Dumez from comment #1) > > The timeouts / crashes seem to go away with the fix for Bug 179067. I'll > > investigate why. > > The assertion that was hit in JSEventListener::jsFunction() is: > // Verify that we have a valid wrapper protecting our function from > // garbage collection. That is except for when we're not in the normal > // world and can have zombie m_jsFunctions. > ASSERT(!m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction); > > So this seems to indicate that the JSEventListener object's wrapper has been > collected too early. > > Normally, such collection is prevented by JSEventTarget's visitor: > void JSEventTarget::visitAdditionalChildren(SlotVisitor& visitor) > { > wrapped().visitJSEventListeners(visitor); > } > > JSServiceWorkerContainer subclasses JSEventTargets and should therefore use > this visiting code. void EventTarget::visitJSEventListeners() is implemented like so: void EventTarget::visitJSEventListeners(JSC::SlotVisitor& visitor) { EventTargetData* data = eventTargetDataConcurrently(); if (!data) return; auto locker = holdLock(data->eventListenerMap.lock()); EventListenerIterator iterator(&data->eventListenerMap); while (auto* listener = iterator.nextListener()) listener->visitJSFunction(visitor); } (In reply to Chris Dumez from comment #3) > (In reply to Chris Dumez from comment #2) > > (In reply to Chris Dumez from comment #1) > > > The timeouts / crashes seem to go away with the fix for Bug 179067. I'll > > > investigate why. > > > > The assertion that was hit in JSEventListener::jsFunction() is: > > // Verify that we have a valid wrapper protecting our function from > > // garbage collection. That is except for when we're not in the normal > > // world and can have zombie m_jsFunctions. > > ASSERT(!m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction); > > > > So this seems to indicate that the JSEventListener object's wrapper has been > > collected too early. > > > > Normally, such collection is prevented by JSEventTarget's visitor: > > void JSEventTarget::visitAdditionalChildren(SlotVisitor& visitor) > > { > > wrapped().visitJSEventListeners(visitor); > > } > > > > JSServiceWorkerContainer subclasses JSEventTargets and should therefore use > > this visiting code. > > void EventTarget::visitJSEventListeners() is implemented like so: > > void EventTarget::visitJSEventListeners(JSC::SlotVisitor& visitor) > { > EventTargetData* data = eventTargetDataConcurrently(); > if (!data) > return; > > auto locker = holdLock(data->eventListenerMap.lock()); > EventListenerIterator iterator(&data->eventListenerMap); > while (auto* listener = iterator.nextListener()) > listener->visitJSFunction(visitor); > } My bet is that the JSServiceWorkerContainer wrapper gets collected and therefore stops marking its event listeners. However, the ServiceWorkerContainer object is still alive is still tries to fire events. Since event listeners are no longer marked, they may already be collected by the time we try to fire the event. (In reply to Chris Dumez from comment #4) > (In reply to Chris Dumez from comment #3) > > (In reply to Chris Dumez from comment #2) > > > (In reply to Chris Dumez from comment #1) > > > > The timeouts / crashes seem to go away with the fix for Bug 179067. I'll > > > > investigate why. > > > > > > The assertion that was hit in JSEventListener::jsFunction() is: > > > // Verify that we have a valid wrapper protecting our function from > > > // garbage collection. That is except for when we're not in the normal > > > // world and can have zombie m_jsFunctions. > > > ASSERT(!m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction); > > > > > > So this seems to indicate that the JSEventListener object's wrapper has been > > > collected too early. > > > > > > Normally, such collection is prevented by JSEventTarget's visitor: > > > void JSEventTarget::visitAdditionalChildren(SlotVisitor& visitor) > > > { > > > wrapped().visitJSEventListeners(visitor); > > > } > > > > > > JSServiceWorkerContainer subclasses JSEventTargets and should therefore use > > > this visiting code. > > > > void EventTarget::visitJSEventListeners() is implemented like so: > > > > void EventTarget::visitJSEventListeners(JSC::SlotVisitor& visitor) > > { > > EventTargetData* data = eventTargetDataConcurrently(); > > if (!data) > > return; > > > > auto locker = holdLock(data->eventListenerMap.lock()); > > EventListenerIterator iterator(&data->eventListenerMap); > > while (auto* listener = iterator.nextListener()) > > listener->visitJSFunction(visitor); > > } > > My bet is that the JSServiceWorkerContainer wrapper gets collected and > therefore stops marking its event listeners. However, the > ServiceWorkerContainer object is still alive is still tries to fire events. > Since event listeners are no longer marked, they may already be collected by > the time we try to fire the event. If so, it would make sense that Bug 179067 fixes the issue since it makes sure the JSServiceWorkerContainer wrapper does not get garbage collected until its implementation object (owned by Navigator) is destroyed. *** This bug has been marked as a duplicate of bug 179067 *** |