350732 - Going back with popup bar on screen truncates scroll view

Status: RESOLVED FIXED

(Camino Graveyard :: Annoyance Blocking, defect)

Description

[Peter Jaros (:peeja)]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1b2) Gecko/20060826 Camino/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1b2) Gecko/20060826 Camino/1.0+

After going back from a page with the popup bar visible, the height of the bar is truncated from the bottom of the browser's scroll view and actual content display.  That is, if the content is longer than the content area's height minus the popup bar's height, there is a scroll bar that stops a popup bar's height from the bottom of the content area.

The extra area is filled with white.  This can be demonstrated by going to http://www.google.com/, then running javascript:void(document.body.style.background="black"), going to the attached URL, and clicking back.

[As a side note, the JavaScript background color change seems to persist at the popup test URL and (helpfully) after navigating back.  Is this a bug in itself which should be filed?]

Reproducible: Always

Steps to Reproduce:
1. Go to a page which is long enough to scroll.
2. Go to a page which brings up the popup bar (such as the attached URL).
3. Navigate back to the original page.

Actual Results:  
The scroll bar and the area for displaying content is truncated from the bottom.

Expected Results:  
The area should not truncate.

Resizing the window clears the symptom.

Comment 1

[froodian (Ian Leue)]

I thought I remembered seeing something similar to this, but I couldn't find it.  I certainly see it, so confirming.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Maybe a little like the original bug Håkan fixed, bug 331430?

Comment 3

[Håkan Waara]

Maybe we need to recalculate the browser view's size also when hiding it, if we don't already. The patch in the bug referenced should hint where this code is.

Comment 4

[Stuart Morgan]

I'm confused; watching [CHBrowserView setFrame:] while reproducing this shows that it's getting called with the right values when loading the page and when going back.

Comment 5

[Sean Murphy]

Attachment: F-Script object browser screenshot for this bug

It seems this bug only occurs *every other time*.  So, when following the reproducible steps listed above, going back from the site containing the popup information bar to google the first time shows the frame drawn incorrectly, but clicking forward and back again corrects the frame height (and this pattern repeats).

After inspecting Camino I'm pretty sure the problem lies somewhere inside the (C++) gecko code.  Using F-Script when the browser frame is incorrect I found a ChildView object who's frame is still set to the wrong size.  I've attached a screenshot of the F-Script object browser for this case.  The view hierarchy goes like this: CHBrowserView -> ChildView -> ChildView (with wrong frame size).  The first two views contain the correct frame height of 745, while the last ChildView still thinks the height is 715, which is what it would be with the popup bar on screen.

I was able to get at that particular ChildView in gdb (which is actually an instance of CHBrowserView if I'm correct) and here's a stack trace of its setFrame method which might isolate the problem.  Since the bug occurs every other time, the first trace is when the frame is drawn correctly, the second is the incorrect case.  (Both are for the exact same page, clicking forward and back again twice in a row)

See both stack traces here: http://pastebin.mozilla.org/818 (didn't want to flood the bug report)

When the ChildView's setFrame is called with the correct height, the trace indicates that it ultimately passed through the BrowserWrapper's setFrame:ResizingBrowserViewIfHidden: method, whereas the trace with the incorrect height originates from somewhere very different.

I don't know the gecko/C++ end of the code very well, so does this help anyone?

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

If this is in Gecko, we probably need to start jumping on the 1.8.1.1 horn as soon as we figure out what exactly is really going on.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

*** Bug 359590 has been marked as a duplicate of this bug. ***

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Moving pink's + from the dupe.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Can someone have a look at this?

Comment 10

[Stuart Morgan]

I spent some time on this before the holidays, and I'll take another look when I get a chance.

Comment 11

[Stuart Morgan]

I dug a bit more; still trying to figure out how we are confusing the history record for the *previous* page (and/or how we aren't confusing it half the time).

Comment 12

[Sean Murphy]

Attachment: Visual example of the problem

This happened to me today, so I figured I might as well grab a screenshot and demonstrate it visually…

You'll notice at the bottom of the browser content there is a gray area which is an artifact corresponding to the page forward in my history.  I clicked on the top link (the "Fort Wayne Journal" one) and that page caused the pop-up blocker to display.  I then moved back to the Google News search, and found what you can see here.  The scrollbars are also truncated.

After resizing the window, to work around the problem, Camino crashed repeatedly (I relaunched and tried the same steps again) with the familiar -[NSWindow sendEvent:] trace:

#0  0x90a5c9c1 in _objc_error ()
#1  0x90a5ca0b in __objc_error ()
#2  0x90a5b060 in _freedHandler ()
#3  0x9334cbe1 in -[NSWindow sendEvent:] ()
#4  0x9333e350 in -[NSApplication sendEvent:] ()
#5  0x93268dfe in -[NSApplication run] ()
#6  0x9325cd2f in NSApplicationMain ()
#7  0x000025a6 in start ()

I'm going to make time tomorrow to look at this bug once again along with Stuart - the more eyes on this the better, since it seems to be relatively easy to run into.  The last time I really sat down and examined it was back in October.  Thankfully, I'm much more familiar with the Core code now than I was when I filed those initial observations in comment 5.

Comment 13

[Sean Murphy]

Ok, I've finally been able to nail why this is happening:

Say you're on a normal page (page "A") that is not displaying the pop-up blocked view and takes up the entire content area with a height of 700.  You then click on a link or otherwise open a new location (page "B") which turns out to be pop-up laden and does utilize the blocker.  Moving to B, page A is saved into the browser history. It's bounds, among many other attributes, are stored as history using nsSHEntry::SetViewerBounds().

When page B is displayed, it throws up the pop-up blocker view and shrinks the CHBrowserView and also its ChildView subviews to make room, basically decreasing all of their heights to 670.  

Then, moving back in history to page A, nsDocShell::RestoreFromHistory() will fetch the stored bounds corresponding to that page using nsSHEntry::GetViewerBounds(), which returns a height of 700.  During the execution of RestoreFromHistory(), it perceives Page B's height to be 670, and will then think there is a mismatch between the two and perform some resizing on the ChildViews (incorrectly).

In nsDocShell.cpp, when the condition on line 5685 is true (meaning the two pages' bounds are not identical), this bug occurrs.

See <http://mxr.mozilla.org/mozilla/source/docshell/base/nsDocShell.cpp#5685>
(newBounds would = 670 and oldBounds = 700 in the example I described).

Now, to explain why this bug was only happening every other time:
We're now moved back onto Page A and discovered that it displayed incorrectly with the truncating problem.  The actual bounds of some ChildViews have a height of 670 in this case, which means they're still accounting for the blocker view.  Clicking forward and archiving Page A into history again stores the incorrect bounds.  Page B then loads, displays the pop-up blocked view.  Moving backward to A this time does not detect a mismatch in the bounds for A & B, because when the height for A was stored it was incorrectly set at 670.  These two matching bounds will not cause RestoreFromHistory() to attempt an explicit resize.

So, as you can read, this one's kinda hard to explain.  I have a logical grasp  inside my head as to what exactly the problem is, and hope to figure out a fix soon.  Basically, we have to perform steps to ensure by the time nsDocShell restores the pages from history, it will fetch and acknowledge the correct bounds for each ChildView.

Comment 14

[Stuart Morgan]

What about hiding the popup blocker on the way into a call to anything that has us moving through history? (I know we currently bypass BrowserWrapper with those calls, but we don't have to.) Is the flicker of redrawing the page we are leaving too annoying?

Comment 15

[Stuart Morgan]

Although, as I think about it more... why doesn't this just automatically fix itself when we do hide the popup blocker later? Are we expanding the host frame *between* when the history code does its fixup and when the view of the previous page is actually put into our view hierarchy?

Comment 16

[Sean Murphy]

(In reply to comment #15)

Yeah, you know what, we are expanding the host frame during the middle of the history code's execution.  When nsDocShell::RestoreFromHistory fetches the bounds the new widget should be, the blocker has not yet been removed and it will obtain the outdated, smaller size and perceive a mismatch.

Stepping through nsDocShell::RestoreFromHistory, the problematic ChildView(s) are actually resized during the SetCurrentURI() call on line 5593 - which is too late and takes place after both newBounds and oldBounds have already been fetched (and this makes newBounds inaccurate).

These variables local to RestoreFromHistory which store each bounds (newBounds, oldBounds) are only used during the if statement on line 5685, which I linked in comment 13.  When the PC reaches this line, if the two sets of bounds are not equal, some resizing is performed and this bug occurs.

Adding a breakpoint in nsDocShell::RestoreFromHistory just after both newBounds and oldBounds have been retreived, and then performing:

(gdb) set newBounds.height = oldBounds.height

You can see that this bug does not happen.  So, to fix this, we'll need to ensure that, by the time newBounds is fetched, the pop-up blocker has already been removed and the returned bounds will contain the correct height.

Comment 17

[Sean Murphy]

I meant to explain above why SetCurrentURI() is actually where the pop-up blocker is removed, since that might not be obvious:

During SetCurrentURI(), nsDocLoader::FireOnLocationChange is called which triggers CHBrowserListener::OnLocationChange since it is registered as one of the listeners.  Then, CHBrowserView will send out the following message when it receives notification of the location change:

- (void)onLocationChange:(NSString*)urlSpec isNewPage:(BOOL)newPage requestSucceeded:(BOOL)requestOK

and finally, this is where -[BrowserWrapper removeBlockedPopupViewAndDisplay] is sent.

This all occurs after nsDocShell::RestoreFromHistory has already obtained newBounds.  A possible fix is to hide the popup blocker sooner than onLocationChange:…, which I think parallels your suggestion in comment 14.

Comment 18

[Stuart Morgan]

(In reply to comment #17)
> A possible fix is to hide the popup blocker sooner than
> onLocationChange:…, which I think parallels your suggestion in comment 14.

Right. In general the reason we don't do this is that it's visually annoying, because the page you are leaving will actually redraw without the popup bar as you leave. It may or may not be better if the next page is from history, and therefore comes up quickly.

Comment 19

[Sean Murphy]

Hmm, hiding the blocker earlier doesn't even appear to be solving the issue.

For instance, visit a site that does not display the blocker (A), then move onto one that does (B).  Go ahead and hide the blocker manually, using its close button.  Then, move back into history and you'll notice that even though the blocker was completely hidden well prior to the restore-from-history calls, this bug occurs.

It's not the CHBrowserView instance that's stuck at the wrong size, but some of the ChildView subviews it contains are never properly restored to the full height.  The call to -[CHBrowserView setFrame:] via -[BrowserWrapper removeBlockedPopupViewAndDisplay] does not propagate to the necessary ChildViews during window->SetSize().  

I'm thinking that some of these views, particularly those corresponding to page A, must be incorrectly set to the reduced height and are not properly restored when the page is displayed again.

I'm still trying to wrap my head around the architecture of how these ChildViews are created and (re)used, but from what I can tell using F-Script and digging into the Cocoa view hierarchy, it gets laid out like this:

Page A
CHBrowserView <0x21049540>
 |-ChildView <0x2ddd370>
   |-ChildView <0x218fba80> *
     |-ChildView <0x21e24060> *

Page B
CHBrowserView <0x21049540>
 |-ChildView <0x2ddd370>
   |-ChildView <0x210be870> *
     |-ChildView <0x22773fb0> *

The first ChildView container in the hierarchy is reused across both Page A and B, and this is the only one which is always set to the correct height.

The views marked with a "*" are new instances each time the page is restored and displayed from history.

Comment 20

[Sean Murphy]

Also, in my previous comment, I meant to explicitly mention that the "*" views are the ones set to the wrong (reduced) height when Page A exhibits this bug.

Comment 21

[Sean Murphy]

Attachment: possible patch idea

Ok, I wanted summarize what's going on, simplify the issue, and explain why the attached patch fixes this bug.  I also hope that explaining the problematic behavior clearly will allow some of the other intelligent minds around here to offer any better suggestions which may exist.

When a page is archived into session history, the frame rect used by that page's root ChildView is stored.  Then, after navigating though history, the restoration code will compare the size of the active root ChildView with the bounds that was archived into the session, and if it detects a mismatch it will go ahead and resize to the active page's frame size.  This is desired behavior; it means that if you resize the browser window and then jump back into history, the page from history will be adjusted to fit the new window size.

We run into a bug because the blocker is hidden after diving into the history and starting the restore.  This means that when the restoration code asks for the current bounds (to compare it with the saved page's) it will end up using the reduced (containing the blocker) height as that value and think there's a mismatch.

This problem only affects history navigation wherein pages are loaded from the cache.  It does not affect the History Menu Bar since that actually fetches the URL again and does not restore from session history.

Fixing this involves ensuring that when the history restoration process begins, it will fetch the full frame size of the current location's content area with no pop-up blocker visible.  Since the restoration is done within Core, accomplishing this is a matter of hiding the blocker prior to asking Core to navigate history.

The patch does two things:
- Remove the blocker prior to history navigation (all other changes in location remove it when we currently do)
- Has the call to removeBlockedPopupViewAndDisplay always set a frame size even if the blocker is already gone.  Even though it simply re-applies the current frame, this value will propagate to certain ChildViews that were not resized when the blocker was actually hidden.

I'm not totally satisfied avoiding the bug in this manner, but it represents the best (and least complicated) technique I could come up with.

> Is the flicker of redrawing the page we are
> leaving too annoying?

The blocker is only hidden early during history navigation.  Doing so doesn't seem to be causing a noticeably different amount of flickering than what is seen currently.  Loading all other pages will remove the blocker when it does now.

+  else {
+    // Even if the view was already hidden, applying the frame again prevents
+    // any size inconsistencies from occurring throughout the view hierarchy.
+    [self setFrame:[self frame] resizingBrowserViewIfHidden:YES];
+  }

The reason for this: Essentially, CHBrowserView (in this case by way of the BrowserWrapper) must receive a setFrame: call after the pages have been loaded from history *if* the blocker was showing on one of them.  We still send removeBlockedPopupViewAndDisplay after a page is loaded from history, and this segment of code is important because it is what causes the frame size to propagate to all appropriate ChildViews on the newly restored page.  In cases of history navigation, the blocker would have already been removed and nothing would allow setFrame: to reach ChildVews corresponding to the restored page (this would all take place before the correct views have been loaded/displayed).

Comment 22

[froodian (Ian Leue)]

Comment on attachment 258697 [details] [diff] [review]
possible patch idea

Nice investigation on this bug, murph.

>   if (mBlockedPopupView) {
>     [mBlockedPopupView removeFromSuperview];
>     [mBlockedPopupView release]; // retain count of 1 from nib
>     mBlockedPopupView = nil;
>     [self setFrame:[self frame] resizingBrowserViewIfHidden:YES];
>     [self display];
>   }
>+  else {
>+    // Even if the view was already hidden, applying the frame again prevents
>+    // any size inconsistencies from occurring throughout the view hierarchy.
>+    [self setFrame:[self frame] resizingBrowserViewIfHidden:YES];
>+  }
> }

If I understand it correctly, the only time this happens is when we've already hidden the blocked popup view, and we're calling it again, which happens from |onLocationChange|.  Since this isn't really related to removing the view (since it's been removed), can you move this call to |onLocationChange|?

Comment 23

[Stuart Morgan]

(In reply to comment #21)
> The patch does two things:
> - Remove the blocker prior to history navigation (all other changes in location
> remove it when we currently do)
> - Has the call to removeBlockedPopupViewAndDisplay always set a frame size even
> if the blocker is already gone.  Even though it simply re-applies the current
> frame, this value will propagate to certain ChildViews that were not resized
> when the blocker was actually hidden.

I don't understand this. If the blocker is removed prior to history navigation, then there should be no problem, because the frame wouldn't be too small by the time the history code gets it. If that works, why would we need the hack of always setting the frame? And if it doesn't work, then why are we doing it at all?

Comment 24

[Sean Murphy]

(In reply to comment #23)

Hiding the blocker before history navigation is a must.  If not hidden by the time we hand off action into Core, nsDocShell::RestoreFromHistory will notice a difference in the frame sizes and perform some resizing (the cause for this bug in the first place).

Hiding the blocker prior to history movement, which (to me) seems absolutely necessary, isn't enough.  Go ahead and visit a site that causes the blocker to display.  Remove the blocker manually, using the close RolloverImageButton.  Then, moving back into history, the frame will still be sized incorrectly even though the blocker was hidden well before Core took control.  The cause is entirely different this time; RestoreFromHistory was not responsible.  Not all ChildViews containing the page from history find out about the correct frame size, and those need to be adjusted explicitly after they're instantiated.

So, the patch attempts to solve two separate, but dependent, issues.  I agree,  it's not an impressive solution, which is why I explained the situation in detail in case there's an approach I missed.

Comment 25

[Stuart Morgan]

(In reply to comment #24)
> Hiding the blocker prior to history movement, which (to me) seems absolutely
> necessary, isn't enough.  Go ahead and visit a site that causes the blocker to
> display.  Remove the blocker manually, using the close RolloverImageButton. 
> Then, moving back into history, the frame will still be sized incorrectly even
> though the blocker was hidden well before Core took control.

I can't reproduce that; I've only ever been able to get truncated pages when I don't manually close the popup blocker first.

Comment 26

[Sean Murphy]

Attachment: same approach, with froodian's suggestion

Updated patch to reflect froodian's advice in comment 22.

Comment 27

[Stuart Morgan]

(In reply to comment #25)
> I can't reproduce that; I've only ever been able to get truncated pages when I
> don't manually close the popup blocker first.

Ah, I think I understand now. Can anyone reproduce the truncation after manual
hiding on a page that doesn't have an onunload popup?

Comment 28

[Stuart Morgan]

Comment on attachment 259587 [details] [diff] [review]
same approach, with froodian's suggestion

This patch doesn't actually handle onunload; that still results in page truncation. Because of that case, doing a pre-navigation removal of the popup blocker won't actually fix the general cause of this bug.

Comment 29

[Stuart Morgan]

Attachment: another approach

I think what we need to do is something like this. It's still kinda hacky, but it should handle all cases.

Comment 30

[Sean Murphy]

Comment on attachment 259599 [details] [diff] [review]
another approach

I'll second this approach; r=me.  We can't easily solve the root cause of this bug so working around it, be it with a hack, might be our only logical choice.  And yeah, this way is much more coherent - I was never happy with my prepareForHistoryNavigation method :(.  I also tested the robustness of the patch with all sorts of popup displaying techniques; it prevented the truncation perfectly every time.

> +- (void)reApplyFrame

Tiny insignificant issue: "reapply" is listed in the dictionary as one word.  For that reason, the method name's camelCase might be better off typed as |reapplyFrame|.

Comment 31

[Stuart Morgan]

Comment on attachment 259599 [details] [diff] [review]
another approach

I'll fix the case on checkin.

Comment 32

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 259599 [details] [diff] [review]
another approach

sr=pink

Comment 33

[Stuart Morgan]

Checked in on trunk and MOZILLA_1_8_BRANCH with s/reApply/reapply/.