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?

No, sorry, I do not. I've just been using the steps to reproduce in comment 0.

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?

That sounds reasonable.

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.

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.

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?

This patch uses an nsISupports pointer instead of a PRInt32. Otherwise, it's basically the exact same patch as before.

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.

Right. This patch uses an nsCOMPtr instead of a plain pointer.

did you forget to qrefresh? This looks like the last patch.

Apparently I did. Here's the real updated patch.

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.

Using the new approach.

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. :)

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.

(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.

I filed bug 564326 to ask Gmail to fix the input control I talked about in comment 6.