Bug 27474 – Crash if the selection is set in a textarea/text input immediately after setting display:none

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ojan Vafai]

summary: webkit crashes if the selection is set in a textarea/text input immediately after setting display:none

Patch + layout test coming soon.

[Ojan Vafai]

Created attachment 33129 [details]
Fixes crashes


---
 8 files changed, 101 insertions(+), 1 deletions(-)

[Darin Adler]

Comment on attachment 33129 [details]
Fixes crashes

This is definitely the right basic approach. And what's here is OK if it's OK for these functions to do nothing when there are pending stylesheets.

But I think it's likely that for website compatibility, these functions need to work even when called early during loading before all the stylesheets are loaded. The code needs to call updateLayoutIgnorePendingStylesheets instead of updateLayout for cases where that needs to work.

I'm going to say review- because of that issue, but I could be convinced to reconsider if you think I'm wrong.

[Ojan Vafai]

I'm not at all familiar with layout updating and pending stylesheets, so I'll trust your judgement that it should call updateLayoutIgnorePendingStylesheets. I'll post a new patch in a bit.

FWIW, calling updateLayoutIgnorePendingStylesheets would be a change in behavior. The old code just calls updateLayout. By moving it up the callstack, we only avoid cases where we would otherwise have crashed (i.e. because the renderer disappeared).

[Darin Adler]

(In reply to comment #3)
> FWIW, calling updateLayoutIgnorePendingStylesheets would be a change in
> behavior. The old code just calls updateLayout. By moving it up the callstack,
> we only avoid cases where we would otherwise have crashed (i.e. because the
> renderer disappeared).

It's true; one could argue that these are two separate issues that perhaps should use two separate bug reports.

[Ojan Vafai]

Created attachment 33303 [details]
Fixes crashes


---
 9 files changed, 132 insertions(+), 29 deletions(-)

[Ojan Vafai]

Changes from previous patch:
-moved some duplicate code into static functions
-s/updateLayout/updateLayoutIgnorePendingStylesheets
-Changed assert to deal with reentrancy in updateLayout
-Changed the updateLayout call in HTMLTextAreaElement::updateFocusAppearance to an assert that the document does not need style recalc or is mid style recalc.

[Darin Adler]

Comment on attachment 33303 [details]
Fixes crashes

> +        Unfortunately, this seems to be
> +        untestable. Loading an external stylesheet and then having an inline
> +        script hit this code did not result in an pending stylesheets.

You should talk to Hyatt. I'm sure he can give you a way to test this.

If we put too many of these calls in we get "flashes of unstyled content" (FOUC).

We need to put these into calls that need to function even when called early, while stylesheets are still loading.

But we should not put it into calls where it's OK if they silently do nothing when called early, while stylesheets are still loading.

There's a risk that by following my request to do the stronger form of updateLayout you will introduce FOUC.

> +static bool isTextFieldWithRendererAfterUpdateLayout(HTMLInputElement* element)

I think the AfterUpdateLayout here is slightly too explicit.

> +    ASSERT(!document()->childNeedsStyleRecalc() || document()->inStyleRecalc());

If the assertion is going to take this form, then maybe we should expose this expression instead of the raw inStyleRecalc value.

r=me

[Ojan Vafai]

Landed as http://trac.webkit.org/changeset/46437

(In reply to comment #7)
> (From update of attachment 33303 [details])
> > +        Unfortunately, this seems to be
> > +        untestable. Loading an external stylesheet and then having an inline
> > +        script hit this code did not result in an pending stylesheets.
> 
> You should talk to Hyatt. I'm sure he can give you a way to test this.

I had talked to Hyatt actually. He suggested having a test case that points to an external stylesheet and then does something that calls updateLayoutIgnorePendingStylesheets. This didn't work. Specifically, there weren't actually any pending stylesheets by the time we got to the updateLayoutIgnorePendingStylesheets call. Hyatt concluded that this "may" be untestable.

I'm happy to put more work into adding a test for this, but I'm probably not the most qualified. Alternately, I can commit a patch that reverts this back to an updateLayout call, but now that I understand what it does, I think you are right that this is a case where we do actually want updateLayoutIgnorePendingStylesheets.

> If we put too many of these calls in we get "flashes of unstyled content"
> (FOUC).
> 
> We need to put these into calls that need to function even when called early,
> while stylesheets are still loading.
> 
> But we should not put it into calls where it's OK if they silently do nothing
> when called early, while stylesheets are still loading.
> 
> There's a risk that by following my request to do the stronger form of
> updateLayout you will introduce FOUC.

Thanks for the explanation.

> > +static bool isTextFieldWithRendererAfterUpdateLayout(HTMLInputElement* element)
> 
> I think the AfterUpdateLayout here is slightly too explicit.

Fixed.

> > +    ASSERT(!document()->childNeedsStyleRecalc() || document()->inStyleRecalc());
> 
> If the assertion is going to take this form, then maybe we should expose this
> expression instead of the raw inStyleRecalc value.

Fixed, although I wasn't able to come up with a great name for this.