Closed Bug 468991 Opened 16 years ago Closed 16 years ago

Customize toolbar sometimes brings up blank sheet (in FF 3.1 and 3.2pre)

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Platform:
PowerPC
macOS
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.1b3

People

(Reporter: mehmet.sahin, Assigned: roc)

References

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 1 obsolete file)

fix
1.44 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081210 Shiretoko/3.1b3pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081210 Shiretoko/3.1b3pre

Hello.

I found following problem with firefox 3.1beta2 + 3.1beta3pre on mac.

I start Firefox - first Window opens. 

After that I open a second window and try to customize the toolbar (in the new second window!!!).

But, there opens only a white window without content.

Only in the first opened window the customize-window opens normally.

Thanks & regards
Mehmet



Reproducible: Always

Steps to Reproduce:
1. Open Firefox 3.1b2 or 3.1b3pre
2. Open a new "second" window
3. Open in the "second" window "customize..." with rightclick on the toolbar.
Actual Results:  
The customize-window is white without content to cusomize the toolbar.

Expected Results:  
customize-window should open in the second with content to customize the toolbar.
Component: General → Toolbars
I can't reproduce this.  I tested with today's Minefield (aka trunk or
mozilla-central) and Shiretoko (aka 1.9.1-branch) nightlies.

Try each of the following in turn, to see if either makes a difference:

1) Run the browser in "Safe Mode" (which runs it without extensions).

   To do this, hold down the Option key while starting the browser.

2) Run with a fresh profile.

   The easiest way to do this (which is reversible) is:

   a) Quit the browser.

   b) Open Terminal.

   c) cd to Library/Application Support/.

   d) Rename the Firefox directory to something like Firefox.old.

   e) Start the browser.
Hello Steven.

I tried it with steps you told me - same problem :-(

here is hardcopy http://www.photouploader.de/pic.php?type=png&f=15397 

Regards
Mehmet
I'm stumped :-(
me too :-( 

THANK YOU anyway...
But see bug 467281.

I *can* reproduce this problem in Thunderbird/Shredder.

And you're welcome :-)
And I (now) can also reproduce this problem in Minefield, the same way I do in Shredder.  See bug 467281 comment #7.
Assignee: nobody → joshmoz
Status: UNCONFIRMED → NEW
Component: Toolbars → Widget: Cocoa
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → cocoa
Version: unspecified → Trunk
Assignee: joshmoz → smichaud
Flags: blocking1.9.1?
Priority: -- → P1
Summary: Can not customize the toolbar, if more than two windows are open (Firefox 3.1b2 + 3.1b3pre) → Customize toolbar sometimes brings up blank sheet (in FF 3.1 and 3.2pre)
thanks for the information and the confirmation of the bug ;-)
I've found a regression range for this problem:

firefox-2008-09-07-02-mozilla-central
firefox-2008-09-08-02-mozilla-central

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-09-07+00%3A00%3A00&enddate=2008-09-08+05%3A31%3A00

I've no idea what patch in this range triggered the problem.  But it
was probably *trigger*, not *cause*.
I am getting this as well.

Problems only started after I manually removed localstore.rdf.
Vlad, it's your patch for bug 451441 (changeset d33ad280baa5) that
triggered this bug (reversing the patch makes it go away).

Do you have *any* idea how that could be? :-)

Note that only OS X uses customizeToolbarSheet.xul (as opposed to
customizeToolbar.xul).  If another OS used customizeToolbarSheet.xul,
it might also have this bug ... or there could be something else going
on.

I'll keep digging.
So if mHasPendingUpdates is false, then in theory all of the events have been passed on to the OS (via FlushPendingInvalidates -> ProcessPendingUpdates -> widget->Invalidate).  I'm not sure why the OS wouldn't draw this though, because as far as gecko knows at this point there's nothing that hasn't been flushed.
The Mac customize toolbar dialog is weird because it's an IFRAME inside a XUL popup panel. Nothing else uses that. The bug is probably related to that, because the view manager treats popup stuff differently and having a view manager inside a popup is weird. See bug 454959 for a similar bug.
Turns out this isn't a widget bug at all.

Every significant operation I tested behaved the same way when the
problem happened as when it didn't:  Exactly the same number of
nsCocoaWindow and nsChildView objects were created, in exactly the
same order.  Mouse-down events always got sent to the same nsChildView
object (and then on to Gecko).  And exactly the same calls were made
to "show" and "hide" nsCocoaWindow and nsChildView objects, with
exactly the same code paths being followed.

So it seems that the blank Customize Toolbar panel (when it appears)
really doesn't have any Gecko objects in it.

I've found a patch that fixes (or works around) this bug, which I'll
post in my next comment.  What I say here will become clearer when you
see it.
Attached patch Fix (or workaround) (obsolete) — — Splinter Review 
Here's a fix for this bug ... or at least a workaround.

I noticed that, when the problem occurs (when the Customize Toolbar
panel is blank), nsViewManager::EnableRefresh() is always called on it
(from nsXULDocument::StartLayout(), with aUpdateFlags set to
NS_VMREFRESH_IMMEDIATE) before the panel becomes visible.  When the
problem doesn't occur, this call always happens after the panel
becomes visible.  Furthermore, r.width and r.height (in
nsXULDocument::StartLayout()) are always '0' when the problem occurs,
but non-zero when it doesn't.

So, when the Customize Toolbar panel is blank, it's because
nsXULDocument::StartLayout() has done an immediate refresh on it while
it was still empty -- as a result it contains no Gecko objects.  And
it stays that way because (for some reason) no further refreshes are
requested.

This bug (or bugs) may have deeper underlying causes.  But others (who
know more about layout and toolkit) are better equipped to find them
than I am :-)

Tryserver builds will follow in a while.
Attachment #352846 - Flags: review?(roc)
I don't know whether to put this in layout or toolkit ... but for the
moment lets say layout (even though the file I patched is in toolkit).
Assignee: smichaud → nobody
Component: Widget: Cocoa → Layout
QA Contact: cocoa → layout
Assignee: nobody → smichaud
Note that the Linux tryserver build may crash on startup (it does on
my machine).  If so, you're seeing bug 469496.
Hello Steven,

I test the Mac-Build from your tryserver. It works!!! ;-) No more blank cusomize toolbar !!!

But another problem... this build tells me (on youtube), that I have no flash installed ?!?!?! (I don't know - perpaps this normal for this build - I am not a developer ;-) )

Greetings 

Mehmet
(In reply to comment #19)

> I test the Mac-Build from your tryserver. It works!!! ;-) No more
> blank cusomize toolbar !!!

Glad to hear it.

> But another problem... this build tells me (on youtube), that I have
> no flash installed ?!?!?!

That's not normal.  But it's an entirely different problem, which I
don't see with my copy of the build.  If it persists, open a new bug.
But before you do so, check to find out if the same problem happens in
other browsers and other versions of Firefox (including ones that
worked previously).
Hi Steven,

I test it with the official 3.2a1pre (Minefield) dated 13-Dec.2008 from ftp.mozilla.org/pub/mozilla.org/firefox/nightly/. Same problem !!! Flash is not played.

But in 3.1b3pre (shiretoko) dated 13-Dec-2008 flash works fine!!!

see enclosed pictures please.

Should I report it as a bug, because you don't have this problem... ?!?!

Link1 - Minefield: http://www.photouploader.de/pic.php?type=png&f=15644

Link2 - Shiretoko: http://www.photouploader.de/pic.php?type=png&f=15645


Regards
Mehmet
(In reply to comment #21)

Actually I now see a similar problem ... but only when I run FF 3.0.4 just before I run my Mac tryserver build (or the 2008-12-12-02-mozilla-central nightly).

This needs to go into another bug.

I'll open one later tonight, after I've got a chance to pin down the details.
okay... thanks & bye !!!

(bye the way... I have seen on the first pic (Minefield) that the java embedding plugin is not load, too)
Mehmet, I've opened bug 469510 on the Flash/Java issue.
Thank you for the information.

I will follow it !!!


Bye
Mehmet
I don't think this is the right fix. Whatever does the call to SetVisibleArea should also be arranging for the right invalidates to be issued.
blocking1.9.1+, we can't ship with this
Flags: blocking1.9.1? → blocking1.9.1+
(In reply to comment #26)

I've tried but failed to extend my analysis further than it's gone
already -- layout is very complex, and I don't (yet) know it well
enough.

But I've found that, when the problem occurs,
nsPresContext::SetVisibleArea() is always called (indirectly) from
DocumentViewerImpl::Show(), and that at this point the dimensions
(i.e. bounds) of the DocumentsViewerImpl's mWindow object (an
nsChildView object) are always empty.  So I'm not sure how (in this
instance) changing the code that calls nsPresContext::SetVisibleArea()
will help.

If you have something else in mind, please be more specific.

In any case, I can't see how my patch (attachment 352846 [details] [diff] [review]) does any
harm.  So I think it will suffice if we can't do any better.
(Following up comment #28)

Note that, when the problem occurs, the panel hasn't yet become
visible.  I've also found that its top-level widget is (at this point)
invisible (it returns FALSE when IsVisible() is called on it).
So when this bug happens, mouse events seem to pass right through the sheet. I get tooltips and status bar changes as I mouse over links in the underlying document. This seems very wrong.
That's a different problem, which also happens in FF 3.0.4.  And (as far as I know) only mouse-moved events are effected.  It also happens whether or not you get a blank sheet.

The easiest way to see it is to arrange a Customize Toolbar panel so that part of it overlays a text-entry box.  When you mouse over that part, you'll see the cursor change shape.

I only first noticed this while debugging this problem.  I'll open a new bug on it soon.
Here's the relevant bit of the call stack for a mouse move while the mouse is over the blank sheet:

(gdb) where
#0  nsViewManager::DispatchEvent (this=0x812dc00, aEvent=0xbfffdef8, aStatus=0xbfffddd8) at /Users/roc/mozilla-trunk/view/src/nsViewManager.cpp:1258
#1  0x01f411c3 in HandleEvent (aEvent=0xbfffdef8) at /Users/roc/mozilla-trunk/view/src/nsView.cpp:167
#2  0x0363c317 in nsChildView::DispatchEvent (this=0x813e7b0, event=0xbfffdef8, aStatus=@0xbfffdeac) at /Users/roc/mozilla-trunk/widget/src/cocoa/nsChildView.mm:1984
#3  0x0362e9b0 in nsChildView::DispatchWindowEvent (this=0x813e7b0, event=@0xbfffdef8) at /Users/roc/mozilla-trunk/widget/src/cocoa/nsChildView.mm:1997
#4  0x0363e39f in -[ChildView mouseMoved:] (self=0x813e850, _cmd=0x95fe7684, theEvent=0xc625e50) at /Users/roc/mozilla-trunk/widget/src/cocoa/nsChildView.mm:3780
#5  0x0363e08e in -[ChildView mouseMoved:] (self=0x812dd70, _cmd=0x95fe7684, theEvent=0xc625e50) at /Users/roc/mozilla-trunk/widget/src/cocoa/nsChildView.mm:3710
#6  0x93b9d3a5 in -[NSWindow sendEvent:] ()
#7  0x0362aa3d in -[NSWindow(MethodSwizzling) nsCocoaWindow_NSWindow_sendEvent:] (self=0xc642a60, _cmd=0x95fcd4b8, anEvent=0xc625e50) at /Users/roc/mozilla-trunk/widget/src/cocoa/nsCocoaWindow.mm:2157
#8  0x03624c56 in -[ToolbarWindow sendEvent:] (self=0xc642a60, _cmd=0x95fcd4b8, anEvent=0xc625e50) at /Users/roc/mozilla-trunk/widget/src/cocoa/nsCocoaWindow.mm:1916
#9  0x93b699fd in -[NSApplication sendEvent:] ()
#10 0x93ac6d0f in -[NSApplication run] ()

ChildView 0x812dd70 is the widget for the root of the content document in the browser window. ChildView 0x813e850 is the widget for the scrolled area in that document. Somehow Appkit seems to be treating the blank sheet as transparent to events. How on earth can we get into that state?
(In reply to comment #31)
> That's a different problem, which also happens in FF 3.0.4.  And (as far as I
> know) only mouse-moved events are effected.  It also happens whether or not you
> get a blank sheet.

You're right, although it's still very disturbing.
Another thing I've noticed is that the blank sheet has rounded corners, like a popup menu, but the normal sheet does not have rounded corners.
Interesting.  I hadn't noticed the rounded corners before.

The Customize Toolbar panel (normal or blank) *is* a popup window.
But in comment #13 you said it (normally) has an IFRAME inside it.
I'll bet this obscures the rounded corners.  And I suppose when you
see a blank sheet the IFRAME is missing.
So I thought this might be related to bug 391984, which added rounded corners to our menus, but it appears not. So how could this sheet be getting rounded corners?
(In reply to comment #35)
> The Customize Toolbar panel (normal or blank) *is* a popup window.
> But in comment #13 you said it (normally) has an IFRAME inside it.
> I'll bet this obscures the rounded corners.  And I suppose when you
> see a blank sheet the IFRAME is missing.

Maybe so, but sheets don't normally have rounded corners, so why does this one?
oh, because it's a panel that only looks like a sheet. Of course :-(
Yeah.  My last three attempts to say that collided with your comments :-)
And notice the beveled edges the panel has when it's not blank.  Also present in FF 3.0.4.
OK, so the panel frame looks like this:
          MenuPopup(panel)(20)@0x136df10 [view=0xc3c4660] next=0x13a719c {12840,3720,38100,24000} [state=80842110] [content=0xbdb28f0] [sc=0x133eb9c]<
            FrameOuter(iframe)(0)@0x12d4d28 [view=0xdb656d0] {0,0,38100,24000} [state=00002010] [content=0xbdb2920]
          >

But the subdocument looks like this:
(gdb) p nsIFrameDebug::DumpFrameTree(0x1648780)
Viewport(-1)@0x1648780 [view=0xdb65a60] {0,0,0,0} [state=00042020] [content=0x0] [sc=0x164864c] pst=:-moz-viewport<
  RootBox(window)(-1)@0x1648818 {0,0,0,0} [state=81c40000] [content=0xdb64070] [sc=0x164888c] pst=:-moz-canvas<

And nsPresContext::mVisibleArea is (0,0,0,0). So it looks like a call to SetVisibleArea with associated reflow should have happened, but didn't, when the outer IFRAME was resized.
Does the view manager inside the frame have an unflushed mDelayedResize?
No. (But thanks, that was worth checking.)
Hmm. We seem to receive the ResizeReflow for the panel's subdocument *before* its InitialReflow!
OK, I've got it. This is hilarious. nsXULDocument::StartLayout does this:

        nsRect r = cx->GetVisibleArea();

        ... various stuff ...
                    vm->EnableRefresh(NS_VMREFRESH_IMMEDIATE);
        ... more stuff ...

        rv = shell->InitialReflow(r.width, r.height);

Now it turns out that the call to vm->EnableRefresh() does some painting (no surprise I guess), and that painting flushes reflow (so that the layout is up to date before we paint). That flushes all the way up the document tree, though, which means we flush the parent document's layout, which reflows the IFRAME and causes the ResizeReflow of the subdocument to happen, passing the IFRAME's width and height, which are duly set into the subdocument's prescontext's mVisibleArea before ResizeReflow bails out due to no root frame (since the initial reflow hasn't happened yet). But now the fun part happens: we then call InitialReflow passing r.width/r.height *which are zero* because we acquired them before the EnableRefresh/ResizeReflow!

I think we just need to move that GetVisibleArea down.
Attached patch fix — — Splinter Review 
Yeah, this works.

I've no idea how to write a test for this. The bug depends on exquisite timing: the XUL in the IFRAME has to finish loading while there is a pending reflow for the parent document.
Assignee: smichaud → roc
Attachment #353366 - Flags: superreview?(dbaron)
Attachment #353366 - Flags: review?(dbaron)
Whiteboard: [needs review]
Comment on attachment 352846 [details] [diff] [review]
Fix (or workaround)

If I'd just looked a bit harder at this patch, I might have seen the bug! But thanks anyway.
Attachment #352846 - Flags: review?(roc) → review-
Nice fix!  Too bad I didn't think to check the result of GetVisibleArea() both before and after the call to EnableRefresh(NS_VMREFRESH_IMMEDIATE).
Attachment #352846 - Attachment is obsolete: true
Attachment #353366 - Flags: superreview?(dbaron)
Attachment #353366 - Flags: superreview+
Attachment #353366 - Flags: review?(dbaron)
Attachment #353366 - Flags: review+
Comment on attachment 353366 [details] [diff] [review]
fix

How about adding a comment that the visible area may have changed as a result of the EnableRefresh we just did, so we need to get it after the EnableRefresh?  (In case somebody later needs GetVisibleArea for something else earlier in the function...)  And probably point to this bug in the comment...

r+sr=dbaron
Whiteboard: [needs review] → [needs landing]
Pushed 71f2ef5bdb40 to trunk
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/988c8faec0a4
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Target Milestone: --- → mozilla1.9.1b3
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
rv:1.9.1b3pre) Gecko/20090122 Shiretoko/3.1b3pre 
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090122 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: