Bug 66659 – REGRESSION (r88115): Uploading a file to mydrive.ch produces an endless busyloop.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

Conditions:
- WebKit r93490 (any version since r91657 will trigger it).
- GtkLauncher was used (but it also happens with my own webkit-based browser)

Steps to reproduce the bug:
1. Go to http://www.mydrive.ch (you need an account, it's free).
2. Log in and press Upload button on the top/left: different upload methods will be shown, select "Standard" method and save.
3. Select a file to upload, and then press Upload button below.
4. The browser enters a neverending busyloop. 

To be noted it didn't happen in r90337. I firt noticed this bug in r91657, so it's somewhere in between.

[Alexey Proskuryakov]

Also happens in Safari on Mac.

This is freezing in HTMLFormControlElement/ShadowRoot::recalcStyle(), so <http://trac.webkit.org/changeset/88115> (convert file <input> to use the new shadow DOM model) seems like a likely culprit.

[Alexey Proskuryakov]

<rdar://problem/10008934>

[Kent Tamura]

Created attachment 105157 [details]
Reduced HTML

[Kent Tamura]

I confirmed r88115 is the culprit, and will post a fix soon.

[Kent Tamura]

Created attachment 105307 [details]
Patch

[Darin Adler]

Comment on attachment 105307 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105307&action=review

> Source/WebCore/rendering/RenderFileUploadControl.cpp:69
> +        bool newDisabled = !theme()->isEnabled(this);
> +        if (button->disabled() != newDisabled)
> +            button->setDisabled(newDisabled);

Could we put this early return inside the setDisabled function instead? It seems wrong for setDisabled to trigger style recalculation if the state is not changing.

[Kent Tamura]

Comment on attachment 105307 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105307&action=review

>> Source/WebCore/rendering/RenderFileUploadControl.cpp:69
>> +            button->setDisabled(newDisabled);
> 
> Could we put this early return inside the setDisabled function instead? It seems wrong for setDisabled to trigger style recalculation if the state is not changing.

Sure.

[Kent Tamura]

Created attachment 105453 [details]
Patch 2

Update setDisabled()

[Darin Adler]

Comment on attachment 105453 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=105453&action=review

> Source/WebCore/html/HTMLFormControlElement.cpp:245
>  void HTMLFormControlElement::setDisabled(bool b)
>  {
> -    setAttribute(disabledAttr, b ? "" : 0);
> +    if (disabled() != b)
> +        setAttribute(disabledAttr, b ? "" : 0);
>  }

I know I was the one who suggested this fix, but now I have to be the one doubting my own suggestion. Since setDisabled is a function exported via the DOM to JavaScript (I did not realize that when I made my suggestion earlier) we need to test what it does in other browsers. For example, does setting it to true overwrite the existing value of the disabled attribute, setting it to the empty string? Also, are there any other side effects of an extra setAttribute call that are detectable from JavaScript?

[Kent Tamura]

(In reply to comment #9)
> I know I was the one who suggested this fix, but now I have to be the one doubting my own suggestion. Since setDisabled is a function exported via the DOM to JavaScript (I did not realize that when I made my suggestion earlier) we need to test what it does in other browsers. For example, does setting it to true overwrite the existing value of the disabled attribute, setting it to the empty string? Also, are there any other side effects of an extra setAttribute call that are detectable from JavaScript?

* The HTML standard says nothing about this case.
    http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#boolean-attribute

* IE9, and Firefox 7 beta behaves same as the curent WebKit. input.disabled=true always updates the "disabled" attribute value.

* The behavior of Opera 11.01 looks same as the "Patch 2".  input.disabled=true doesn't update the "disabled" attribute value if the "disabled" attribute already has a value.

I think we had better follow IE9 and Firefox.  So I'll withdraw the second patch and set r? to the first patch.

[Darin Adler]

Comment on attachment 105307 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105307&action=review

>>> Source/WebCore/rendering/RenderFileUploadControl.cpp:69
>>> +        if (button->disabled() != newDisabled)
>>> +            button->setDisabled(newDisabled);
>> 
>> Could we put this early return inside the setDisabled function instead? It seems wrong for setDisabled to trigger style recalculation if the state is not changing.
> 
> Sure.

I suggest we add a “why” comment explaining why we need to check the disabled state instead of unconditionally calling setDisabled.

[Kent Tamura]

Thank you for reviewing.

(In reply to comment #11)
> I suggest we add a “why” comment explaining why we need to check the disabled state instead of unconditionally calling setDisabled.

ok, I'll add a comment and will land the patch.

[Kent Tamura]

Committed r94045: <http://trac.webkit.org/changeset/94045>