552493 - Hang on select-all in large textbox (which repeats if I unfocus & focus Firefox)

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Visit URL
 2. Click "Edit Attachment As Comment"
 3. Click inside the textbox
 4. Press Ctrl+A to select all

ACTUAL RESULTS:
 - Firefox hangs for ~10 seconds or more, maxing out CPU while it's hanging.
 - If I focus another window while Firefox is hanging, and then I re-focus Firefox after it's stopped hanging (i.e. by clicking its titlebar), it will hang again.

EXPECTED RESULTS:
 - No (or less) hanging
 - If there's a (minor) hang, I should be able to alt-tab away during it, and then alt-tab back without kicking the hang off again.

I get ACTUAL RESULTS in Firefox 3.6 and mozilla-central nightlies:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.2pre) Gecko/20100310 Namoroka/3.6.2pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a3pre) Gecko/20100315 Minefield/3.7a3pre

I get EXPECTED RESULTS on Firefox 3.5.8: (virtually *no* hang on select-all, and a ~5 sec hang if I click again in the textbox to cancel the selection):
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.8) Gecko/20100202 Firefox/3.5.8

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  I would have called this a dup of the editor <br> bug (complete with the focus/blur stuff), except for the claim of a regression from 3.5.x.  I've certainly seen identical behavior in Gecko 1.9.1 and earlier.

Daniel, do you have time to narrow down a regression range for what you see?  Things shouldn't have regressed here; they should be as bad as they ever were...

Comment 2

[Timothy Nikkel (:tnikkel)]

This seems to be the regression range.

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d3240cd1c610&tochange=1886b176f000

The best candidate seems to be bug 338209.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Even if I set the spellchecker underline as none by "ui.SpellCheckerUnderlineStyle", it's too slow, so, nsTextFrame::CombineSelectionUnderlineRect() might be slow.

Is nsTextFrame::GetSelectionDetails() returns all selections (including all selections which are outside of the textframe)?

Comment 4

[Daniel Holbert [:dholbert]]

Sorry bz -- I didn't notice comment 1 'til now.  I'll check whether my regression range matches Timothy's later tonight.

Comment 5

[Daniel Holbert [:dholbert]]

Yup, I have the same regression range as Timothy. (Thanks for tracking that down, Timothy!)

WORKS  (Select-all happens almost instantly)
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090402 Minefield/3.6a1pre

BROKEN (Select-all takes seconds to complete & hangs my CPU):
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090403 Minefield/3.6a1pre

(and the regression pushlog is what Timothy gave in comment 2)

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Seems that nsTextFrame::GetSelectionDetails() is too slow but it's called many times.

nsTextFrame::SetSelectedRange() is called many times by nsTypedSelection::selectFrames() and nsTypedSelection::SelectAllFramesForContent() either directly or via nsTextFrame::SetSelected().
http://mxr.mozilla.org/mozilla-central/ident?i=SetSelectedRange

When I changed to:

> @@ -5077,29 +5077,32 @@ nsTextFrame::SetSelectedRange(PRUint32 a
>        if (details) {
>          anySelected = PR_TRUE;
>          DestroySelectionDetails(details);
>        } else {
>          f->RemoveStateBits(NS_FRAME_SELECTED_CONTENT);
>        }
>      }
>  
> +    if (aType & SelectionTypesWithDecorations) {
>        // We may need to reflow to recompute the overflow area for
>        // spellchecking or IME underline if their underline is thicker than
>        // the normal decoration line.
>        PRBool didHaveOverflowingSelection =
>          (f->GetStateBits() & TEXT_SELECTION_UNDERLINE_OVERFLOWED) != 0;
>        nsRect r(nsPoint(0, 0), GetSize());
>        PRBool willHaveOverflowingSelection =
>          aSelected && f->CombineSelectionUnderlineRect(presContext, r);
>        if (didHaveOverflowingSelection || willHaveOverflowingSelection) {
>          presContext->PresShell()->FrameNeedsReflow(f,
>                                                     nsIPresShell::eStyleChange,
>                                                     NS_FRAME_IS_DIRTY);
>        }
> +    }
> +
>      // Selection might change anything. Invalidate the overflow area.
>      f->InvalidateOverflowRect();
>  
>      f = static_cast<nsTextFrame*>(f->GetNextContinuation());
>    }
>  
>    // Scan remaining continuations to see if any are selected
>    while (f && !anySelected) {

Then, "select all" becomes fast. However, clearing the selection is still slow because following code calls GetSelectionDetails() very many times.

>     if (aSelected) {
>       f->AddStateBits(NS_FRAME_SELECTED_CONTENT);
>       anySelected = PR_TRUE;
>     } else { // we need to see if any other selection is available.
>       SelectionDetails *details = f->GetSelectionDetails();
>       if (details) {
>         anySelected = PR_TRUE;
>         DestroySelectionDetails(details);
>       } else {
>         f->RemoveStateBits(NS_FRAME_SELECTED_CONTENT);
>       }
>     }

Comment 7

[Timothy Nikkel (:tnikkel)]

In doing the bisecting I noticed that clearing the selection was slow in both builds where selecting all was fast and builds where it was slow. So clearing the selection being slow is a separate problem.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Is nsTextFrame::GetSelectionDetails() returns all selections (including all
> selections which are outside of the textframe)?

No.

> Seems that nsTextFrame::GetSelectionDetails() is too slow but it's called many
> times.

GetSelectionDetails is more or less O(M*log(N)) where N is number of selections and M is number of child nodes.  In our case M is length of the text.  That comes from the position compares it ends up having to do.  Nixing <br> will make M be much smaller.  Faster position compares in the DOM would also make this faster.

See also the discussion in bug 449167.

In the meantime, is the diff in comment 6 correct in the sense that we could reasonably take that fix (esp. on branches)?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #8)
> In the meantime, is the diff in comment 6 correct in the sense that we could
> reasonably take that fix (esp. on branches)?

I think so. The normal selection change doesn't cause the overflow rect changes.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'd say we should do that, then.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

ok, I'll post the patch in a couple of hours.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

We should check the overflow rect only when the selection type which has decoration line is changed.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/523d0f8e0926