Bug 33983 – Setting HTMLSelectElement.length to remove elements is broken if the page has (or ever had) DOM mutation events registered

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

Open the test page and click the checkbox - the select is empty.  Then reload the page, hit 'Inspect Element' anywhere on the page, and click the checkbox again.  This time it only removes ~half the <option>s from the <select>, rounded down.  This appears to be fairly consistent.

I'm guessing that some inspector code is causing the HTMLSelectElement's listItems to be recalculated in the middle of removing the children - probably by the inspector touching some field that triggers it.

[James Robinson]

Created attachment 47167 [details]
Repro page

[James Robinson]

Created attachment 47170 [details]
Repro page with mutation events

This isn't a bug with the inspector, it's actually with DOM mutation events.  On WebKits old enough for the inspector to use DOM mutation events it shows up when the inspector is open.  On ToT it only shows up if the page explicitly registers a mutation event (it doesn't matter what the body of the handler does).

[James Robinson]

Created attachment 47260 [details]
Patch

[Alexey Proskuryakov]

-        const Vector<Element*>& items = listItems();
+        const Vector<Element*> items = listItems();

This fixes vector iteration. What guarantees that elements don't get deleted, making pointers in the vector stale?

[Darin Adler]

Comment on attachment 47260 [details]
Patch

> -        const Vector<Element*>& items = listItems();
> +        const Vector<Element*> items = listItems();

The const here should also be removed.

If mutation events are involved, then it's not enough to copy the vector. We also have to ref() the element pointers because there's no guarantee the mutation event handlers will not eliminate the last reference to one of the list items.

So this should be a Vector<RefPtr<Element> > and the code should be written to put the items into that vector.

I'm going to say review- because it seems that if we're going to change this we should address that problem as well as this more obvious one.

[Alexey Proskuryakov]

It probably goes without saying, but please try to make a test case for the additional issue. To force garbage collection, you can use the technique from e.g. <http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/security/detached-sandboxed-frame-access.html>.

[James Robinson]

Created attachment 47460 [details]
patch

This patch uses the Vector<RefPtr<..> > approach and adds a bunch more tests.  The algorithm is very simple, when the 'length' is set to something shorter than the current length, the code first builds a list of all option elements past the new 'length' and adds them to a list to be removed.  It then iterates through the list and attempts to call removeChild() on each of them, assuming that they still have a parent.  The following actions are all possible during the second iteration:

1.) A mutation event handler removes an element that is already scheduled to be deleted (covered by select-set-length-with-mutation-remove.html).  In this case the remove just doesn't happen.
2.) A mutation event handler reorders options after the truncation has begun by causing an option that was scheduled to be removed to be earlier in the list (covered by select-set-length-with-mutation-reorder.html).  In this case the items scheduled to be removed are still removed regardless of their new position.
3.) A mutation event handler moves an option that is scheduled to be removed to be a child of a different select element (covered by select-set-length-with-mutation-reparent.html).  In this case the element is removed from its new parent.

The odd behavior in case 3 matches what Firefox 3.5 and IE8 do.  WebKit ToT crashes on some of these tests without this patch applied.  With this patch, we match Firefox 3.5 and IE8 on all cases.

[James Robinson]

The DOM level 3 events spec (which covers mutation events) is a bit disappointing as to what should happen here.  In particular, it says:

Many single modifications of the tree can cause multiple mutation events to be dispatched. Rather than attempt to specify the ordering of mutation events due to every possible modification of the tree, the ordering of these events is left to the implementation.

http://www.w3.org/TR/DOM-Level-3-Events/#events-mutationevents

[Kent Tamura]

Comment on attachment 47460 [details]
patch

It's not so easy to check whether the test results in this patch are correct or not. We usually print "PASS" if a test passes.
I recommend to rewrite the tests in script-tests form.  You can see a lot of examples in LayoutTests/fast/forms/script-tests/. You don't need to write if(window.layoutTestController)... and function log(msg) for script-tests.

> -        for (size_t listIndex = 0; listIndex < items.size(); listIndex++) {
> -            if (items[listIndex]->hasLocalName(optionTag) && optionIndex++ >= newLen) {
> -                Element *item = items[listIndex];
> +        for (size_t i = 0; i < items.size(); ++i) {

I don't think the variable name "i" is wrong, but it seems there are no reason to shorten the variable name.

[James Robinson]

Created attachment 49766 [details]
Patch

[James Robinson]

Thanks for looking at this.  I apologize for letting this bug languish for so
long.

I rewrote the tests using script-tests.  They are much cleaner now.  I left the
loop index as 'i' as it seems that is far more common in the WebKit codebase
and the verbosity didn't seem to do much other than make the loop look more
confusing than it really is.

[Kent Tamura]

The patch looks good.
I would give r=me if I was a reviewer :-)

[Dimitri Glazkov (Google)]

Comment on attachment 49766 [details]
Patch

tests ftw!

[James Robinson]

Comment on attachment 49766 [details]
Patch

Clearing flags on attachment: 49766

Committed r55557: <http://trac.webkit.org/changeset/55557>

[James Robinson]

All reviewed patches have been landed.  Closing bug.