808466 - Changing selection sometimes doesn't repaint immediately

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't have good steps to reproduce yet, but sometimes when I'm starting a selection on page, we don't seem to repaint the page immediately. I suspect a SchedulePaint is missing somewhere along the code paths that change the selection.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I've actually been hitting this a lot on Mac.  :(  I also haven't found a good way to reproduce, though.

Comment 2

[Matt Woodrow (:mattwoodrow)]

I don't know the selection code at all, but it might be worth just throwing a few SchedulePaint calls in at the entry points to selection.

Attempts to reproduce this haven't been all that successful so far :)

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Let's keep an eye on this and if STR are found we'll want to check and make sure this wasn't present in earlier versions than 18.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Lukas Blakk [:lsblakk] from comment #3)
> Let's keep an eye on this and if STR are found we'll want to check and make
> sure this wasn't present in earlier versions than 18.

I'm pretty sure this is a DLBI regression.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

When the bug occurs, it looks a lot like we simply fail to paint the window right away, and if you wait for a bit doing nothing the window does repaint and the selection change is rendered.

But I don't see how this could be happening. nsTextFrame::SetSelectedRange calls InvalidateFrame() on every affected text frame. That should schedule a paint. Selection::SelectAllFramesForContent calls InvalidateFrameSubtree on every non-text frame, which should also schedule a paint.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I posted a message to dev-platform in case someone there can reproduce the bug.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't have any ideas for tracking this down other than running SPS and analyzing the profile.

Comment 9

[Alex Keybl [:akeybl]]

We'll keep this on the tracking list for upcoming releases (at least temporarily), but we won't be in a position to take a fix here in our final beta of Firefox 18 (nor would we want to).

Comment 10

[Colby Russell :crussell (no longer Mozilla-ing)]

What kind of delay are we talking about?  A noticeable fraction of a second?  If so, I noticed something along those lines last week when doing selectiony things in XUL multiline text areas, and it should be reproducible.  But if so, this would be present at least as far back as Gecko 14.  I'll see if I can remember what it was.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Colby Russell :crussell from comment #10)
> What kind of delay are we talking about?  A noticeable fraction of a second?
> If so, I noticed something along those lines last week when doing selectiony
> things in XUL multiline text areas, and it should be reproducible.  But if
> so, this would be present at least as far back as Gecko 14.  I'll see if I
> can remember what it was.

The delay varies. It could be a noticeable fraction of a second, or more.

Comment 12

[Colby Russell :crussell (no longer Mozilla-ing)]

In comment #10, Colby Russell :crussell wrote:
> I noticed something along those lines last week

Nevermind.  I'm pretty sure the delay I was seeing was just the event click type discrimination logic.  Sorry.

Comment 13

[Mounir Lamouri (:mounir)]

Since the beginning of December, there are a few random oranges related to focus (like bug 660224). Could that be related?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, possibly.

I just encountered a bit of jank while selecting and grabbed an SPS profile, but nothing shows up in its responsiveness metrics except for a cycle collection, which probably wasn't long enough to cause the problem I saw.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I seem to get this fairly often browsing http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml and selecting text in it.

Comment 16

[Mayank Bansal]

http://people.mozilla.com/~bgirard/cleopatra/#report=5bddbb7898f6cccb1ff1d5a9de9f3bf5bf922aad

i very frequently get this bug as i have a habit of constantly selecting and dragging the selected text while i browse a page. This is on a old profile, with a fresh FF start, google search, go to search result page, select text and try to drag it around.  Profiler is enabled from the start.

Comment 17

[Mayank Bansal]

Much better profiles : 

http://people.mozilla.com/~bgirard/cleopatra/#report=3d9dfeceec483db9c8c94447755084ef45bc182f


http://people.mozilla.com/~bgirard/cleopatra/#report=6896f210edf051e16d31db350e02a53982874d10

quite reliable STR :

1. Open https://bugzilla.mozilla.org/show_bug.cgi?id=808466 . scroll up and down, select unselect drag copy text.
2. In a background tab, open https://bugzilla.mozilla.org/show_bug.cgi?id=801555
3. Switch to the step2 tab.
4. Scroll down to comment 17 (where the about support is given)
5. Now start playing with the text there. Select it, unselect it, copy text. Do it for 15-20 seconds. Eventually the bug occurs.


The profiles i posted have the bug in the last 3-4 seconds from the end.

Comment 18

[Mayank Bansal]

fully repro steps :

1.select some text on this page by double clicking. 
2. Right click on the text. A context menu appears. *Move mouse inside the context menu*. 
3. Now click elsewhere on the page. The selected text will not be unselected. And you will not be able to select new text.

selected text can be unselected quickly by scrolling the mouse.

Perhaps bug 813579 is related ?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The profiles aren't all that useful after all :-(. But the steps in comment #18 look very interesting and I can definitely reproduce that bug! I'll focus on fixing that first.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Note that with the steps in comment #18, you get the behavior I've been seeing where anything that causes a repaint of the window and causes any new selection to suddenly become visible.

Thanks myankleoboy1, this is great.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

pres->PresShell()->ScheduleViewManagerFlush() in nsIFrame::SchedulePaint does not get called.

nsTextFrameThebes::SetSelectedRange does get called, and calls nsIFrame::InvalidateFrame.

I'm guessing that some ancestor frame has NS_FRAME_DESCENDANT_NEEDS_PAINT set even though there is no paint scheduled.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix?

This seems to fix the issue in comment #18, for me. I haven't had time to see whether other issues are fixed.

Tryserver builds will appear here:
https://tbpl.mozilla.org/?tree=Try&rev=263cb06773c1
I would appreciate some testing by people who can reproduce this bug.

Comment 23

[Mayank Bansal]

Attachment: probable another case

Comment 24

[Mayank Bansal]

tried the win32 build from here :
http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/[e-mail]-263cb06773c1/try-win32/?C=S;O=A

Using this i cant repro comment 18 . That seems to be fixed.
Also,   bug 813579  seems to be fixed by this too , :)


the comment 23 is still repro. But thats probably another bug, though the STR are similar.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm wondering whether this patch fixes the problems you described in comments #16 and #17 as well.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 695989 [details] [diff] [review]
fix?

Review of attachment 695989 [details] [diff] [review]:
-----------------------------------------------------------------

I think the problem here is that when we call InvalidateFrameInternal with aFrame being a popup frame, we set NS_FRAME_DESCENDANT_NEEDS_PAINT on its parent and then bail out. We do a SchedulePaint for the popup, which clears NS_FRAME_DESCENDANT_NEEDS_PAINT from all the popup's frames, but a stray NS_FRAME_DESCENDANT_NEEDS_PAINT will be left on the popup's parent frame. This means subsequent InvalidatePaintInternal calls on other descendants of the popup frame's parents can be swallowed without causing SchedulePaints.

This is a pretty serious bug. It may be too late for 18 but we'll see.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 695989 [details] [diff] [review]
fix?

Review of attachment 695989 [details] [diff] [review]:
-----------------------------------------------------------------

Fishing for reviews during this holiday season :-)

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 695989 [details] [diff] [review]
fix?

Review of attachment 695989 [details] [diff] [review]:
-----------------------------------------------------------------

Fishing for reviews during this holiday season :-)

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm going away tonight and tomorrow so if this patch gets review, I'd appreciate it if someone could land it for me.

Comment 30

[Timothy Nikkel (:tnikkel)]

Comment on attachment 695989 [details] [diff] [review]
fix?

That makes sense.

Comment 31

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d01ebbc522d8

Comment 32

[Mats Palmgren (inactive)]

Isn't the stuff you do before the loop now the same as the stuff
you do inside it, so now you could write:
  parent = aFrame;
  do {
     stuff...
     parent = nsLayoutUtils::GetCrossDocParentFrame(parent);
  } while (as before);

Comment 33

[Timothy Nikkel (:tnikkel)]

Hmm, except we add NS_FRAME_NEEDS_PAINT to aFrame, but NS_FRAME_DESCENDANT_NEEDS_PAINT to its parent and ancestors.

Comment 34

[Mats Palmgren (inactive)]

Ah right, so I guess you could write it like so:
  parent = aFrame;
  bits = NS_FRAME_NEEDS_PAINT;
  do {
     if (aHasDisplayItem) {
       parent->AddStateBits(bits);
     }
     more stuff...
     bits = NS_FRAME_DESCENDANT_NEEDS_PAINT;
     parent = nsLayoutUtils::GetCrossDocParentFrame(parent);
  } while (as before);

Alternatively, instead of 'bits':
     parent->AddStateBits(parent == aFrame ? NS_FRAME_NEEDS_PAINT : 
                                             NS_FRAME_DESCENDANT_NEEDS_PAINT);

Comment 35

[Mayank Bansal]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> I'm wondering whether this patch fixes the problems you described in
> comments #16 and #17 as well.

comment 16 and comment 17 are the same as comment 18 , just with more noise and guessing STR.

Probably stating the obvious, but this patch also solves the equivalent problem of
1. Go to a tab with some text
2. Right click and move mouse in the context menu.
3. Try to select some text. You cant.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 695989 [details] [diff] [review]
fix?

[Approval Request Comment]
Bug caused by (feature/regressing bug #): DLBI
User impact if declined: Mysterious occasional jank-like failure to repaint in response to user input (briefly)
Testing completed (on m-c, etc.): landed on inbound
Risk to taking this patch (and alternatives if risky): Low risk. The patch only changes behavior in a rare case and the change is conservative (call SchedulePaint more often).
String or UUID changes made by this patch: None

Comment 37

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/d01ebbc522d8

Comment 38

[bhavana bajaj [:bajaj]]

Comment on attachment 695989 [details] [diff] [review]
fix?

We have crossed the cut-off time to take patches in FF beta unless its a blocker issue.Only approving for aurora at this time.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/da93f5dda913

Comment 40

[Mayank Bansal]

IMHO, this should be included in FF18 release also, as it is a source of general user annoyance.

Comment 43

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on FF 19 beta 6.
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130212082553

Comment 44

[Ada [:adalucinet]]

Verified as fixed on FF 20 RC (Build ID: 20130326150557). 
This issue is still reproducible on Mac OSX 10.8 and Linux as in comment 18, please see bug 855705.