Closed Bug 562447 Opened 10 years ago Closed 10 years ago

when navigating in gmail sometimes get a screen full of white

Categories

(Core :: Layout: Form Controls, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5
Tracking Status
blocking2.0 --- beta1+

People

(Reporter: tnikkel, Assigned: ehsan)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files, 5 obsolete files)

Open a "long" conversation in gmail, about 5 screens worth or more (expand collapsed messages if you need), scroll to the bottom of the page, click the "Back to x" link on the bottom left where x is "Inbox" or a label name, where ever you just came from. You get a screen full of white; you are actually returned to the correct place but you are scrolled further down the page, below any of the content so it is just blank.

Local bisection pinned this on bug 353539.
I've been seeing this for a few days but keep forgetting to file it. :-/
Given that bug 353539 just added a "scroll into view" call, I wonder if it just exposed a variation of one of bug 559993 or bug 539949 in a situation where we previously didn't make a "scroll into view" call.
Nope, doesn't seem to be related to bug 559993 or bug 539949 at all as rolling back to before 526394 and applying bug 353539 still shows the problem.
Timothy, do you have a reduced test case for this?
Component: Editor → Layout: Form Controls
QA Contact: editor → layout.form-controls
No, sorry, I do not. I've just been using the steps to reproduce in comment 0.
Keywords: testcase-wanted
So, I think I know what's happening here.  Gmail adds a very strange input tag to the DOM, like this:

<input style="position: absolute; width: 10px; height: 10px; left: -50px; top: 25175px;">

And for some reason calls focus() on it, and then blurs it later on.  In nsTextControlFrame::SetFocus, we post a ScrollSelectionIntoView event <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#274> to scroll the text control's selection into view.  When gmail's script is done, this event triggers and causes this broken behavior.

I think one possible fix here is to provide a private selection API to cancel any existing scroll events, so that we can cancel the event when we're later blurred.

Would that be a good fix to look into?
Attached file Test case
This test case reproduces this problem.  I tested it in all browsers that matter (Minefield, Fx3.6, Chrome, Chromium dev channel), plus Safari, and Minefield is the only one failing this test.

Note that if you reorder the focus and scrollTo calls, the page will end up being scrolled to the bottom in all those browsers, and that's the expected behavior.  The test case for this bug should probably take that case into consideration as well.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
I verified that this patch both fixes the test case I created earlier, and the actual bug on gmail.  I made sure that we only cancel a scroll event if it's ours.  That said, I'm not too proud of the reinterpret_cast's there, but I couldn't think of a better way to do this.
Attachment #443289 - Flags: superreview?(roc)
Attachment #443289 - Flags: review?(bzbarsky)
Also, if reinterpret_cast is the way to go here, should I be using PRInt64 instead, for 64-bit compatibility sake?  I was really hesitant to add four bytes overhead per text control, and I was trying to save yet another four bytes...
Why can't you just pass around a pointer to the nsISupports for the runnable?
Attached patch Patch (v2) (obsolete) — Splinter Review
This patch uses an nsISupports pointer instead of a PRInt32.  Otherwise, it's basically the exact same patch as before.
Attachment #443289 - Attachment is obsolete: true
Attachment #443400 - Flags: superreview?(roc)
Attachment #443400 - Flags: review?(bzbarsky)
Attachment #443289 - Flags: superreview?(roc)
Attachment #443289 - Flags: review?(bzbarsky)
It seems like you'll be holding a dangling pointer here after the scroll has finished? Perhaps the cached scroll event should be an nsCOMPtr?

GetPendingScrollEvent should definitely addref.
(In reply to comment #13)
> It seems like you'll be holding a dangling pointer here after the scroll has
> finished? Perhaps the cached scroll event should be an nsCOMPtr?
> 
> GetPendingScrollEvent should definitely addref.

This is why I didn't like to pass the pointer around.  This pointer should never be dereferenced.  Actually, it should never be treated as a pointer.  Its only use is to serve as a token for comparison against the active scroll event.

The reason why I don't want this pointer to be manipulated in nsTextControlFrame is that it's pointing to an nsIRunnable which is being managed in nsSelection.cpp by a nsRevocableEventPtr, and exposing it to the outside world is opening a can of worms, because nsRevocableEventPtr doesn't have any ownership concept built into it:

<http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h#426>
In fact, maybe we should just store it as a void*, to make sure that it'll be treated as such?
Using a dangling pointer this way is unsafe even if you only do comparisons. For example, if the event is freed and then latter another scrolling event allocated at the same address, we'll cancel the wrong event.

An nsCOMPtr would prevent the event from being deallocated, avoiding this problem.
Attached patch Patch (v2.1) (obsolete) — Splinter Review
Right.  This patch uses an nsCOMPtr instead of a plain pointer.
Attachment #443400 - Attachment is obsolete: true
Attachment #443532 - Flags: superreview?(roc)
Attachment #443532 - Flags: review?(bzbarsky)
Attachment #443400 - Flags: superreview?(roc)
Attachment #443400 - Flags: review?(bzbarsky)
did you forget to qrefresh? This looks like the last patch.
Attached patch Patch (v2.1) (obsolete) — Splinter Review
Apparently I did.  Here's the real updated patch.
Attachment #443532 - Attachment is obsolete: true
Attachment #443547 - Flags: superreview?(roc)
Attachment #443547 - Flags: review?(bzbarsky)
Attachment #443532 - Flags: superreview?(roc)
Attachment #443532 - Flags: review?(bzbarsky)
Actually I think a better approach would be to have ScrollSelectionIntoView return the event that was triggered, if any, via an out parameter. That would prevent us from making mistakes where we accidentally cancel an existing scroll-into-view event.
Hmm, have you considered just making the call to ScrollIntoView on focus() happen as an asynchronous event itself? And in that asynchronous event, check whether the element you're trying to scroll into view actually still has focus, and if it doesn't, just bail out?
(In reply to comment #21)
> Hmm, have you considered just making the call to ScrollIntoView on focus()
> happen as an asynchronous event itself? And in that asynchronous event, check
> whether the element you're trying to scroll into view actually still has focus,
> and if it doesn't, just bail out?

I think this is a better approach.  Thanks for suggesting it.

Actually, I decided to use a revocable event, and on entering SetFocus, I just revoke the old event if any is around.
Comment on attachment 443547 [details] [diff] [review]
Patch (v2.1)

r- per discussion.
Attachment #443547 - Flags: review?(bzbarsky) → review-
Attached patch Patch (v3) (obsolete) — Splinter Review
Using the new approach.
Attachment #443547 - Attachment is obsolete: true
Attachment #443900 - Flags: superreview?(roc)
Attachment #443900 - Flags: review?(bzbarsky)
Attachment #443547 - Flags: superreview?(roc)
Comment on attachment 443900 [details] [diff] [review]
Patch (v3)

Actually, this version of the patch doesn't require sr.  Switching the review request to roc based on his smaller review queue.  :)
Attachment #443900 - Flags: superreview?(roc)
Attachment #443900 - Flags: review?(roc)
Attachment #443900 - Flags: review?(bzbarsky)
Why do we need mWeakFrame here since we revoke the event?

Shouldn't we be revoking the event on blur?
Er, never mind about the second comment.
Attached patch Patch (v3.1)Splinter Review
(In reply to comment #26)
> Why do we need mWeakFrame here since we revoke the event?

We don't.  This new version of the patch removes the usage of the weak frame.

> Shouldn't we be revoking the event on blur?

We do.  For blur, SetFocus is called with aOn set to PR_FALSE.
Attachment #443900 - Attachment is obsolete: true
Attachment #443974 - Flags: review?(roc)
Attachment #443900 - Flags: review?(roc)
I filed bug 564326 to ask Gmail to fix the input control I talked about in comment 6.
See Also: → 564326
http://hg.mozilla.org/mozilla-central/rev/cfa870bf8e38
Flags: in-testsuite+
Target Milestone: --- → mozilla1.9.3a5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
blocking2.0: ? → beta1+
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.