Bug 164814 - Add more assertions to ElementQueue diagnose a bug
Summary: Add more assertions to ElementQueue diagnose a bug
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 162029
  Show dependency treegraph
 
Reported: 2016-11-15 19:37 PST by Ryosuke Niwa
Modified: 2016-12-06 21:32 PST (History)
7 users (show)

See Also:


Attachments
Tightens assertions (2.30 KB, patch)
2016-11-15 19:40 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (2.80 KB, patch)
2016-11-15 21:16 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Set m_invoking in release builds (1.31 KB, patch)
2016-12-05 21:36 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-11-15 19:37:25 PST
We seem to be sporadically hitting an assertion in CustomElementReactionStack::ElementQueue on our bots.

I don’t quite understand how this is possible.

We swap the vector at the beginning of the function before start invoking on elements.
So the only way m_elements can be not empty is if an item was added to m_elements while we’re calling invokeAll:

inline void CustomElementReactionStack::ElementQueue::invokeAll()
{
#if !ASSERT_DISABLED
    TemporaryChange<bool> invoking(m_invoking);
#endif
    Vector<Ref<Element>> elements;
    elements.swap(m_elements);
    for (auto& element : elements) {
        auto* queue = element->reactionQueue();
        ASSERT(queue);
        queue->invokeAll(element.get());
    }
    ASSERT(m_elements.isEmpty());
}

But I’ve added an assertion for m_invoking so that m_invoking is false when we’re calling add:
inline void CustomElementReactionStack::ElementQueue::add(Element& element)
{
    ASSERT(!m_invoking);
    // FIXME: Avoid inserting the same element multiple times.
    m_elements.append(element);
}
which isn't firing...
Comment 1 Ryosuke Niwa 2016-11-15 19:40:21 PST
Created attachment 294918 [details]
Tightens assertions
Comment 2 Yusuke Suzuki 2016-11-15 19:42:51 PST
Comment on attachment 294918 [details]
Tightens assertions

r=me
Comment 3 Ryosuke Niwa 2016-11-15 21:16:13 PST
Created attachment 294923 [details]
Patch for landing
Comment 4 WebKit Commit Bot 2016-11-15 22:18:21 PST
Comment on attachment 294923 [details]
Patch for landing

Clearing flags on attachment: 294923

Committed r208785: <http://trac.webkit.org/changeset/208785>
Comment 5 WebKit Commit Bot 2016-11-15 22:18:26 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Ryosuke Niwa 2016-12-05 21:36:34 PST
Reopening to attach new patch.
Comment 7 Ryosuke Niwa 2016-12-05 21:36:37 PST
Created attachment 296267 [details]
Set m_invoking in release builds
Comment 8 WebKit Commit Bot 2016-12-06 15:02:23 PST
Comment on attachment 296267 [details]
Set m_invoking in release builds

Clearing flags on attachment: 296267

Committed r209426: <http://trac.webkit.org/changeset/209426>
Comment 9 WebKit Commit Bot 2016-12-06 15:02:28 PST
All reviewed patches have been landed.  Closing bug.