574690 - Title bar(window without body) appears at the upper left corner of monitor screen for a split second when starting Minefield and restore previous size of browser

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100625 Minefield/3.7a6pre ID:20100625040119
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100625 Minefield/3.7a6pre ID:20100625040119

In Windows7 Classic Style (not Aero).
Title bar(window without body) appears at the upper left corner of monitor screen for a split second when starting Minefield and restore previous size of browser.

This does not happen in Aero style.

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2.Please watch a screen carefully

Actual Results:
Window without body appears at the upper left corner of monitor screen for a split second when starting Minefield. And the browser is restored to the position/size of the last time.

Expected Results:
Title bar without body should not appear.

Regression window:

Works:
http://hg.mozilla.org/mozilla-central/rev/522df66198cf
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100624 Minefield/3.7a6pre ID:20100624185700

Fails:
http://hg.mozilla.org/mozilla-central/rev/51bd519736c4
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100624 Minefield/3.7a6pre ID:20100624221810

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=522df66198cf&tochange=51bd519736c4

Comment 1

[Alice0775 White]

And it happens on Windows XP.

Comment 2

[Alice0775 White]

And it happens not only start up, it also happens when open any dialog such as "Download Manager" and "About Minefield" and new browser.

Comment 3

[Rob Arnold [:robarnold]]

Jim and I think that it's being caused by painting child windows that have loading documents. No clear solution yet.

Comment 4

[Jim Mathies [:jimm]]

Attachment: DoResetWidgetBounds stack

The values here are completely messed up. This probably has to do with initialization of the view early in the creation of the window. In the past, children wouldn't be effected by this, but now that a view holds a top level, calls like this can have side effects.

Comment 5

[Timothy Nikkel (:tnikkel)]

I would think that by the time LoadComplete is called views would be setup already in either case.

Comment 6

[[not reading bugmail]]

We used to see this before when it was the only window open, and doing stuff like trying to restore the window size, etc fixed it.  But I think we were talking about session restore then or not restoring windows that were closed if they were dialogs.  I don't know if that is also at the heart of this bug.. but something to think about.

Comment 7

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 8

[Rob Arnold [:robarnold]]

Comment on attachment 454382 [details] [diff] [review]
patch v.1

Why do we not set mHasValidSize anywhere? It defaults to PR_FALSE so we have a bunch of dead code.

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: New patch: Prevent DocumentViewer from showing in certain situations

This is a new approach. We prevent the DocumentViewer from showing the window if it's a top level window and the document shell tree owner is an nsIXULWindow. In this situation we know the nsXULWindow will take care of showing it.

Comment 10

[Bas Schouten (:bas.schouten)]

Attachment: New patch: Prevent DocumentViewer from showing in certain situations v2

Corrected to NULL-check mParentWidget.

Comment 11

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

I think you should check treeItem->GetParent instead of looking at mParentWidget.

Comment 12

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

I mean, instead of checking the widget type of 'mWindow'.

Comment 13

[Bas Schouten (:bas.schouten)]

Attachment: Call Show if our nsIXULWindow will not

Updated as per IRC discussion.

Comment 14

[Bas Schouten (:bas.schouten)]

Pushed http://hg.mozilla.org/mozilla-central/rev/7ad36412775e.

Comment 15

[Alice0775 White]

It is not fixed on latest hourly
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100627 Minefield/3.7a6pre ID:20100627215239

[STR]
1. Start Minefield in Classic style (non Aero)
2. Open Library (Ctrl + Shift + B)

Comment 16

[Jim Mathies [:jimm]]

(In reply to comment #15)
> It is not fixed on latest hourly
> Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre)
> Gecko/20100627 Minefield/3.7a6pre ID:20100627215239
> 
> [STR]
> 1. Start Minefield in Classic style (non Aero)
> 2. Open Library (Ctrl + Shift + B)

confirmed, I see it too. We'll have to do some more debugging. This patch definitely fixed the size/show problem for the main browser window.

Alice can you confirm that part of it?

Comment 17

[Bas Schouten (:bas.schouten)]

This bug is fixed, indeed there is still a bug with other windows, filed bug 575184. This probably doesn't need to block beta 1.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Didn't the iid here need to change?

Comment 19

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

Yes, sorry! Bas, please rev the IID.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks like this patch has been regressed bug 575281 which prevents us from using D&D of tabs. Their content isn't shown anymore.

Comment 21

[Jim Mathies [:jimm]]

Reopening since this seems the best place to post a fix.

Comment 22

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 23

[Jim Mathies [:jimm]]

Comment on attachment 457474 [details] [diff] [review]
patch v.1

I'm backing out the original changes as they weren't needed. The core problem here is that when chrome loads, nxXULWindow fires off a Size() and a Move() to the widget using persisted data. The Size event triggers a gui size mode event, which propagated up to document viewer, triggering an early call to Show. This is causing a number of display anomalies when we first open windows. 

What I've done is just reorder the calls in nxXULWindow's OnChromeLoaded so that Move() is made before Size(). (Since Move doesn't trigger a Show() in document viewer like Size(), the problem goes away.)

Comment 24

[Boris Zbarsky [:bzbarsky]]

> I'm backing out the original changes 

Sort of.  You forgot to remove the nsIXULWindow include, and for some reason you're removing curlies that were there all along and that should stay.  ;)

It looks like we used to call Move() before Size() before the fix for bug 113283.  Did that fix just accidentally change the ordering, or was that a necessary part of fixing whatever it was trying to fix?

Comment 26

[Jim Mathies [:jimm]]

(In reply to comment #24)
> > I'm backing out the original changes 
> 
> Sort of.  You forgot to remove the nsIXULWindow include, and for some reason
> you're removing curlies that were there all along and that should stay.  ;)

I'm planning on just backing out the original patch.

> It looks like we used to call Move() before Size() before the fix for bug
> 113283.  Did that fix just accidentally change the ordering, or was that a
> necessary part of fixing whatever it was trying to fix?

Looks like Dan moved the size up so that it would be on top of the if(mIntrinsicallySized) block, then plopped position in below it. I don't see any reason why that can't be reordered. The original LoadSizeFromXUL did position, then size. If it breaks something, I'm sure we'll hear about it soon enough.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 457474 [details] [diff] [review]
patch v.1

OK, r=me if the original patch for this bug is backed out instead of the partial backout in this patch landing.

Let's hope you're right about "soon enough"....

Comment 28

[Jim Mathies [:jimm]]

(In reply to comment #27)
> Comment on attachment 457474 [details] [diff] [review]
> patch v.1
> 
> OK, r=me if the original patch for this bug is backed out instead of the
> partial backout in this patch landing.
> 
> Let's hope you're right about "soon enough"....

I pushed it to try, we'll see if anything shows up.

Comment 29

[Jim Mathies [:jimm]]

Attachment: patch w/o backed out code

Comment 30

[Jim Mathies [:jimm]]

Attachment: patch w/o backed out code

Comment 31

[Jim Mathies [:jimm]]

Attachment: refreshed w/code backout

Comment 32

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/d4aafb90ff96
http://hg.mozilla.org/mozilla-central/rev/7124132f0506

Comment 33

[Christophe Calvez]

Hi,
it is indeed better, but when opening a new window from a maximized window, I can see a first painting of the new window on it's "not maximized" position, then it is maximized.
No problem when the new window doesn't have to be maximized.

Comment 34

[Gary [:streetwolf52]]

still seeing the same problem. A mini caption bar appears when I hide the Fx menu and the Fx button isn't displayed.  I see a white rectangle when the Fx button is displayed.

Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100716 Minefield/4.0b2pre - Build ID: 20100716040830

Comment 35

[Jim Mathies [:jimm]]

(In reply to comment #33)
> Hi,
> it is indeed better, but when opening a new window from a maximized window, I
> can see a first painting of the new window on it's "not maximized" position,
> then it is maximized.
> No problem when the new window doesn't have to be maximized.

Unrelated to this, please file a separate bug if you feel the issue is serious. (Also, you might check 3.6 too, to see if the behavior has changed since that release.)

Comment 36

[aja+bugzilla]

Problem is back on XP, too, with
Mozilla/5.0 (Windows; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100716
Minefield/4.0b2pre ID:20100716040830

First nightly I've seen this problem reappear (since right before beta1?,
iirc).

Comment 37

[Gary [:streetwolf52]]

Should have mentioned that I did not have this problem for a long time.  It was fixed at one point in the past and then with today's nightly it reappeared.

Comment 38

[Jim Mathies [:jimm]]

(In reply to comment #37)
> Should have mentioned that I did not have this problem for a long time.  It was
> fixed at one point in the past and then with today's nightly it reappeared.

Gary, can you reproduce in a clean profile, no add-ons? (Which isn't to say I'll drop this, add-ons may be triggering an early size event to document viewer, so I might have to revisit that area of the code with an additional fix.)

Comment 39

[Jim Mathies [:jimm]]

(In reply to comment #34)
> still seeing the same problem. A mini caption bar appears when I hide the Fx
> menu and the Fx button isn't displayed.  I see a white rectangle when the Fx
> button is displayed.
> 
> Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100716
> Minefield/4.0b2pre - Build ID: 20100716040830

This doesn't sound like the same as the app start problem, can you clarify a bit how this relates to this bug?

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #39)
> This doesn't sound like the same as the app start problem, can you clarify a
> bit how this relates to this bug?

It happens for me on my regular profile, but it didn't with that profile's safe-mode. (The window DID move from the left half of the screen over to the right when it drew in all the chrome widgets and stuff in safemode, but I don't know if that's something to do with safemode.)

I'll try to narrow down the list of extensions to find out what's causing it.

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #40)
> (In reply to comment #39)
> > This doesn't sound like the same as the app start problem, can you clarify a
> > bit how this relates to this bug?
> 
> It happens for me on my regular profile, but it didn't with that profile's
> safe-mode. (The window DID move from the left half of the screen over to the
> right when it drew in all the chrome widgets and stuff in safemode, but I don't
> know if that's something to do with safemode.)
> 
> I'll try to narrow down the list of extensions to find out what's causing it.

Looks like it's Firebug 1.6X.0a17 in my case.

Comment 42

[Jim Mathies [:jimm]]

Attachment: frame before maximize

This shows the problem related to maximized state I'm seeing. If you max the browser, close it down completely, then open again, you might see a normalized window frame briefly before the window maximizes.

I tested in 3.6, but it seems that version doesn't restore the window show state correctly. So I'm not sure if this is a regression or not. Can anyone else confirm?

Comment 43

[Jim Mathies [:jimm]]

(In reply to comment #42)
> I tested in 3.6, but it seems that version doesn't restore the window show
> state correctly. So I'm not sure if this is a regression or not. Can anyone
> else confirm?

Actually, scratch that, I had a minefield window minimized on the taskbar. :) I can confirm I do not see the initial frame on 3.6.

Comment 44

[Gary [:streetwolf52]]

Seems to be caused by many add-ons to one degree or another.  Some add-ons will produce the mini title bar very, very fast while others will keep them on the screen longer so you notice it. 

One add-on that keeps it on the screen the longest is LastPass 1.69.1. This might be because LP logs into it's server.  Also Adblock Plus shows the mini title bar but not as long. 

IMO all add-ons might cause this behavior.  Some you might not notice becuase it happens so fast, others you do.

Comment 45

[Gary [:streetwolf52]]

created new bug report

https://bugzilla.mozilla.org/show_bug.cgi?id=579421

Comment 46

[Boris Zbarsky [:bzbarsky]]

This caused bug 598673.

Comment 47

[Jim Mathies [:jimm]]

(In reply to comment #26)
> (In reply to comment #24)
> > > I'm backing out the original changes 
> > 
> > Sort of.  You forgot to remove the nsIXULWindow include, and for some reason
> > you're removing curlies that were there all along and that should stay.  ;)
> 
> I'm planning on just backing out the original patch.
> 
> > It looks like we used to call Move() before Size() before the fix for bug
> > 113283.  Did that fix just accidentally change the ordering, or was that a
> > necessary part of fixing whatever it was trying to fix?
> 
> Looks like Dan moved the size up so that it would be on top of the
> if(mIntrinsicallySized) block, then plopped position in below it. I don't see
> any reason why that can't be reordered. The original LoadSizeFromXUL did
> position, then size. If it breaks something, I'm sure we'll hear about it soon
> enough.

(In reply to comment #27)
> Comment on attachment 457474 [details] [diff] [review]
> patch v.1
> 
> OK, r=me if the original patch for this bug is backed out instead of the
> partial backout in this patch landing.
> 
> Let's hope you're right about "soon enough"....

soon enough! I'm going to back this out. But first I want a better fix.

Comment 48

[Jim Mathies [:jimm]]

Attachment: widget fix

I need to run through all the test cases and bugs to be sure this gets them all. Looks good so far based on initial testing.

Comment 49

[Jim Mathies [:jimm]]

Attachment: widget fix

This patch includes:

changeset:   55271:044722b9b0d4
parent:      47785:7124132f0506
user:        Jim Mathies <[e-mail]>
date:        Sat Oct 09 16:02:52 2010 -0500
summary:     backout of 7124132f0506

tag:         patch
tag:         tip
parent:      55270:904a556a15f2
user:        Jim Mathies <[e-mail]>
date:        Sat Oct 09 16:08:26 2010 -0500
summary:     Bug 574690 - Suppress spurious widget Show events during window initialization in nsXULWindow's OnChromeLoaded.

Comment 50

[[e-mail]]

Comment on attachment 482082 [details] [diff] [review]
widget fix

> NS_METHOD nsWindow::Show(PRBool bState)
> {
> #if defined(MOZ_SPLASHSCREEN)
>   // we're about to show the first toplevel window,
>   // so kill off any splash screen if we had one
>   nsSplashScreen *splash = nsSplashScreen::Get();
>   if (splash && splash->IsOpen() && mWnd && bState &&
>       (mWindowType == eWindowType_toplevel ||
>        mWindowType == eWindowType_dialog ||
>        mWindowType == eWindowType_popup))
>   {
>     splash->Close();
>   }
> #endif
> 
>+  if (mSuppressShow) {
>+    mSuppressedState = bState;
>+    return NS_OK;
>+  }
Shouldn't this go before the splash screen check, in case we need both?

>+    if (mWindow)
>+      mWindow->SuppressShow(PR_FALSE);
>+
>     if (mShowAfterLoad) {
>       SetVisibility(PR_TRUE);
>       // At this point the window may have been closed during Show(), so
>       // nsXULWindow::Destroy may already have been called. Take care!
>     }
This worries me. On platforms supporting SuppressShow it's possible that the SuppressShow(PR_FALSE) call triggers script that closes the window. On the other hand on platforms not supporting SuppressShow the SetVisibility call might trigger script that closes the window...

Comment 51

[Jim Mathies [:jimm]]

(In reply to comment #50)
> >+    mSuppressedState = bState;
> >+    return NS_OK;
> >+  }
> Shouldn't this go before the splash screen check, in case we need both?

I'll move it up.

> 
> >+    if (mWindow)
> >+      mWindow->SuppressShow(PR_FALSE);
> >+
> >     if (mShowAfterLoad) {
> >       SetVisibility(PR_TRUE);
> >       // At this point the window may have been closed during Show(), so
> >       // nsXULWindow::Destroy may already have been called. Take care!
> >     }
> This worries me. On platforms supporting SuppressShow it's possible that the
> SuppressShow(PR_FALSE) call triggers script that closes the window. On the
> other hand on platforms not supporting SuppressShow the SetVisibility call
> might trigger script that closes the window...

Tracking if Destroy has been called could address this. I'll post a new patch tomorrow with the changes.

Comment 52

[Jim Mathies [:jimm]]

Comment on attachment 482082 [details] [diff] [review]
widget fix

This wasn't complete either. With script popups we want the show to be delayed past OnChromeLoaded since we still haven't set the correct size and position yet. This patch only addressed persisted attribs on the window.

Comment 53

[Jim Mathies [:jimm]]

Hmm, now that we've removed all sub widgets, I don't think nsDocumentViewer needs to call Show on the widget at all.

Reopening this since I'm planning on backing out the changes that landed.

Comment 54

[Jim Mathies [:jimm]]

Attachment: don't call show patch

I'm going to run this through try tonight. I'm pretty sure this is safe since the base window's visibility is handled by other code  in nsXULWindow, nsWindowWatcher, & nsFocusManager.

Comment 55

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/5c698ec1841c
http://hg.mozilla.org/mozilla-central/rev/060d498e6c8b
http://hg.mozilla.org/mozilla-central/rev/a9e92cabc714

Comment 56

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336