Bug 33983 - Setting HTMLSelectElement.length to remove elements is broken if the page has (or ever had) DOM mutation events registered
Summary: Setting HTMLSelectElement.length to remove elements is broken if the page has...
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2010-01-21 17:57 PST by James Robinson
Modified: 2010-03-04 15:29 PST (History)
3 users (show)

See Also:

Repro page (271 bytes, text/html)
2010-01-21 17:59 PST, James Robinson
no flags Details
Repro page with mutation events (328 bytes, text/html)
2010-01-21 18:42 PST, James Robinson
no flags Details
Patch (5.66 KB, patch)
2010-01-22 19:14 PST, James Robinson
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
patch (15.75 KB, patch)
2010-01-26 16:23 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (19.56 KB, patch)
2010-03-01 17:41 PST, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2010-01-21 17:57:32 PST
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.
Comment 1 James Robinson 2010-01-21 17:59:51 PST
Created attachment 47167 [details]
Repro page
Comment 2 James Robinson 2010-01-21 18:42:48 PST
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).
Comment 3 James Robinson 2010-01-22 19:14:48 PST
Created attachment 47260 [details]
Comment 4 Alexey Proskuryakov 2010-01-22 23:33:26 PST
-        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?
Comment 5 Darin Adler 2010-01-23 10:29:11 PST
Comment on attachment 47260 [details]

> -        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.
Comment 6 Alexey Proskuryakov 2010-01-23 11:03:26 PST
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>.
Comment 7 James Robinson 2010-01-26 16:23:47 PST
Created attachment 47460 [details]

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.
Comment 8 James Robinson 2010-01-27 16:00:48 PST
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.

Comment 9 Kent Tamura 2010-02-10 08:21:48 PST
Comment on attachment 47460 [details]

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.
Comment 10 James Robinson 2010-03-01 17:41:52 PST
Created attachment 49766 [details]
Comment 11 James Robinson 2010-03-01 17:43:41 PST
Thanks for looking at this.  I apologize for letting this bug languish for so

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.
Comment 12 Kent Tamura 2010-03-01 19:26:48 PST
The patch looks good.
I would give r=me if I was a reviewer :-)
Comment 13 Dimitri Glazkov (Google) 2010-03-04 14:15:38 PST
Comment on attachment 49766 [details]

tests ftw!
Comment 14 James Robinson 2010-03-04 15:29:35 PST
Comment on attachment 49766 [details]

Clearing flags on attachment: 49766

Committed r55557: <http://trac.webkit.org/changeset/55557>
Comment 15 James Robinson 2010-03-04 15:29:40 PST
All reviewed patches have been landed.  Closing bug.