536926 - Clicking link in iframe causes contents of iframe to be erased

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Timothy Nikkel (:tnikkel)]

Paul Wright reported this issue in https://bugzilla.mozilla.org/show_bug.cgi?id=525751#c14

This existed before bug 525751 existed. The regression range is

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=90d3e6d2cbb9&tochange=4430cae50dad

The only things that stick out to me in that range are bug 495920 and bug 178324.

Comment 1

[Timothy Nikkel (:tnikkel)]

Hmmm, I'm getting a different regression range on Linux.

Comment 2

[Timothy Nikkel (:tnikkel)]

Seems to be some platform specific things going on here, as I double checked the regression range on windows from comment 0, and found that on Linux roc's removing widgets from iframes on 2009-07-21 causes this bug (with the fix for bug 525751 backported).

In builds before the problem appeared it seems as though we black out the whole game iframe, but in builds with the problem we just black out the smaller iframe with the button. The former doesn't look bad because we are used to that when navigating between pages, but the latter is a little jarring because we don't expect the in game buttons to be inside a separate iframe. So it seems like we are getting bitten here because we are "blacking out" the whole page for a shorter period of time and thus seeing the "blacking out" of just the button inside the iframe.

If you open the game iframe top level, then the problem doesn't appear.

I not really sure what we want to do here.

Comment 3

[Timothy Nikkel (:tnikkel)]

Hitting ctrl-tab during the brief period when the button iframe is blacked out will not do anything. There does not appear to be any such problem in builds that don't have the button iframe gets blacked out problem.

Comment 4

[Timothy Nikkel (:tnikkel)]

I was assuming the buttons in the game where inside their own iframes because that was the most obvious explanation, but they aren't. So we must be getting an invalidate for a region containing the clicked button sometime before the next page loads.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: zipped testcase

Here is a reduced testcase. yahoo.com may load too quickly for you to see the effect, so replace with a different link.

I see the yahoo link disappearing before the "this is some other content" disappears and the next pages loads.

Regression range for this testcase for Windows is in comment 0. On Linux the regression range is the one in comment 2.

Comment 6

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

(In reply to comment #4)
> So we must be getting an invalidate for a region containing the clicked button
> sometime before the next page loads.

It makes sense that we could have invalidated an area and have a pending OS repaint event that happens to arrive after we've stopped painting the previous page.

The question is, why didn't the state change that stops us from painting the previous page also invalidate the whole page?

Comment 7

[Timothy Nikkel (:tnikkel)]

Here is what happens. In nsDocShell::CreateContentViewer we call FirePageHideNotification for the old page. This calls nsFocusManager::WindowHidden via DocumentViewerImpl::PageHide which removes focus from the focused content. This trigger the problematic invalidate. Back in nsDocShell::CreateContentViewer we create the new viewer and call Embed, which sets its new view as the rootview of the subdocument. The new view doesn't have any frames until the paint suppression timer fires. Once nsDocShell::CreateContentViewer finishes (and whatever else) we serve the invalidate and paint. In nsSubDocumentFrame::BuildDisplayList the subdocView is the new root view and so we just paint the fallback background.

When the document is not in an iframe the paint request is targeted directly at the rootview (instead of the subdocument's parent document's rootview), and we can continue to draw the old page until DocumentViewerImpl::Hide is called on the old page via the DocumentViewerImpl::Show call on the new page.

In nsSubDocumentFrame::BuildDisplayList the view for the old page is still there, it is the next sibling of the rootview of the new page; it doesn't get removed until DocumentViewerImpl::Hide is called on the old page. Using that view to paint when the first view doesn't have a frame solves the painting problem. The is similar to what we do with toplevel documents and results in less time when we draw the fallback background color.

The issue of key presses getting lost of comment 3 has a different cause. PresShell::HandleEvent retargets the event at the rootview of the new page in the iframe and we get to this code at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6243

    // key and IME events go to the focused frame
    if (NS_IS_KEY_EVENT(aEvent) || NS_IS_IME_EVENT(aEvent) ||
        NS_IS_CONTEXT_MENU_KEY(aEvent) || NS_IS_PLUGIN_EVENT(aEvent)) {

where we then get the focused content from the focus manager, but it is null since WindowHidden set it to null. So we set mCurrentEventContent to the root content (which doesn't have a frame yet) and then we

    if (GetCurrentEventFrame()) {
      rv = HandleEventInternal(aEvent, aView, aEventStatus);
    }

which does nothing, and we've now reached the end of the function with the event unhandled. Adding a check for !GetCurrentEventFrame() to

      if (!mCurrentEventContent || InZombieDocument(mCurrentEventContent)) {
        rv = RetargetEventToParent(aEvent, aEventStatus);

fixes the problem.

And finally moving the code

  // inform the window so that the focus state is reset.
  NS_ENSURE_STATE(mDocument);
  nsPIDOMWindow *window = mDocument->GetWindow();
  if (window)
    window->PageHidden();

from DocumentViewerImpl::PageHide (which is called while we still expect to draw the page for a while) and putting it into DocumentViewerImpl::Hide fixes both the paint and key presses getting lost problems. Also, when loading a new page we don't see the focus get removed the link we just clicked a half second before the new page appears. This code was added with the focus rewrite and seems to be the source of the problem.

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: Call WindowHidden when we actually hide the window

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: Make sure events don't get lost

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: Try to draw the old subdocument by looking for the old view

I also noticed that we were still getting IsPaintingSuppressed even though we don't use that anymore.

Comment 11

[Neil Deakin]

(In reply to comment #8)
> Created an attachment (id=419809) [details]
> Call WindowHidden when we actually hide the window

I'm not sure it's possible or safe to fire events from DocumentViewer::Hide. If it is, I think I'd have to test these changes a lot more.

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to comment #11)
> (In reply to comment #8)
> > Created an attachment (id=419809) [details] [details]
> > Call WindowHidden when we actually hide the window
> 
> I'm not sure it's possible or safe to fire events from DocumentViewer::Hide. If
> it is, I think I'd have to test these changes a lot more.

Ok, we don't need this patch if we take the other two, it would just stop the focus being visibly removed just before the next page shows up.

Comment 13

[Neil Deakin]

Is this ready to be checked in?

Comment 14

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/64072ad0d749
http://hg.mozilla.org/mozilla-central/rev/2258a632387e

I don't have plans to get this in 1.9.2 due to it being a relatively minor problem, unless there are strong feelings to do so.