Closed Bug 322074 Opened 17 years ago Closed 15 years ago

couple of xul tags hide window on unload

Categories

(Core :: Layout, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: bernd_mozilla, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: regression, testcase)

Attachments

(3 files, 2 obsolete files)

see attached testcase, at least if loaded locally hitting a couple of times the reload button hides the window, It is still listed in the task bar but clickiing on it does not bring it up (this is with current trunk). The only way to terminate the app then is the taskmanager.
Attached file testcase (obsolete) —
loading the testcase and hitting back is also "efficient"
confirmed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051230 Firefox/1.6a1. Pretty efficient DoS.
Attached file shorter testcase
Attachment #207312 - Attachment is obsolete: true
we hit
###!!! ASSERTION: failed to get the nsIScriptGlobalObject. bug 95465?: 'boundGlobal', file d:/moz_src/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp, line 439
Summary: couple of xul tags hide window on reload → couple of xul tags hide window on unload
Boris do you know who deals with that kind of assert?
Blocks: xangle
Um... hyatt?  Or maybe bryner, jst, or me.  :(
Severity: normal → critical
Keywords: regression
My guess is that Roberts checkin to reenable the overflow events from bug 305120 triggers something with the xbl handler of the arrowscrollbox
http://landfill.mozilla.org/mxr-test/seamonkey/source/toolkit/content/widgets/scrollbox.xml#57
I'd suspect bug 305120, but it was checked into the 1.8 branch on 2005-08-31, and I can't reproduce this there.
(In reply to comment #11)
Not anymore, but also in branch it regressed. 
Between 1.8b4_2005083106 and 1.8b4_2005083120.


Yeah, that matches the landing of bug 305120 on the branch. So what fixed it there? Or maybe this depends on something not in my clean branch profile?
Does the trunk also change at these days (1.8b5_2005092913 and 1.8b5_2005092922) the bevahior back?
(In reply to comment #15)
Yes, you're right :(
Between 1.9a1_2005092912 and 1.9a1_2005092922.
In that case there must have been a second regression.

Second regression: between 1.9a1_2005100709 and 1.9a1_2005100805.

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-10-07+08%3A00%3A00&maxdate=2005-10-08+05%3A00%3A00&cvsroot=%2Fcvsroot






pointing to bug 301411 and bug 311223
Backing out bug 311223 fixes the issue here.

- // Don't use nsRect's operator== here, since it returns true when
-  // both rects are empty even if they have different widths and we
-  // have cases where that sort of thing matters to us.

It seems that generating the overflow event for the xbl handler is one of them. There is also a different rendering in both cases...

from bug 311223
> it's a bad interaction with horizontal scrolling of trees
trees also react on the overflow event http://landfill.mozilla.org/mxr-test/seamonkey/source/toolkit/content/widgets/tree.xml#602
So yeah.  What we have here is a vicious cycle, as far as I can tell.  We're reflowing, which resizes views and all... and posts an overflow event.  In this event we change the collapsed state of some nodes (in the arrowscrollbox binding).  That triggers another reflow, which resizes things and posts an underflow event.  And so forth.

When we get into the code bug 311223 touched, we have either:

(gdb) p oldDimensions
$7 = {x = 0, y = 0, width = 0, height = 0}
(gdb) p aRect
$8 = (const nsRect &) @0xbfffdfa0: {x = 0, y = 0, width = 84, height = 0}

or 

(gdb) p oldDimensions
$14 = {x = 0, y = 0, width = 0, height = 0}
(gdb) p aRect
$15 = (const nsRect &) @0xbfffde10: {x = 0, y = 0, width = 0, height = 98}

depending on whether we're underflowing or overflowing, so we bail out early.  Not sure why not bailing out there would help.

It's a good thing we do async reflow and restyles; otherwise this would just stack overflow and crash.  :(

roc, any idea what we could do here?

Also ccing some folks who might know how we could make the scrollbox binding not screw us over like this.
Blocks: 311223
Flags: blocking1.9a1?
(In reply to comment #20)
>Also ccing some folks who might know how we could make the scrollbox binding
>not screw us over like this.
Aha, it turns out that the grid somehow enforces a max height on the scrollbox, thus making it go into vertical overflow, which, as it (in this case) is a horizontal scrollbox it should ignore.
Attached patch Lazy fix (obsolete) — Splinter Review
Note: hwaara was also working improving things in bug 222274.
Comment on attachment 207821 [details] [diff] [review]
Lazy fix

Odd, I thought I had it working with this, but now I can't reproduce it.
Attachment #207821 - Attachment is obsolete: true
It's not helping that the under/overflow events don't have a true event target.
Comment on attachment 207313 [details]
shorter testcase

Note: in future, where possible, it would help for the document to be attached in the XUL mime type, and also be given the XUL global stylesheet.
I can't reproduce this bug on ff trunk 20060105, Mac OS X.

However, the vicious cycle of reflows is something I noticed when working on bug 222274. In that patch I've tried to minimize the times we set the collapse attribute and cause unnecessary overflows, etc.
I can reproduce just fine with a current trunk (well, Friday night) debug Linux Firefox build (using "shorter testcase").
Flags: blocking1.9a1?
Flags: blocking1.9?
Flags: blocking-firefox3?
Neil, can you take this?
Assignee: nobody → enndeakin
Flags: blocking1.9? → blocking1.9+
So I'm actually having trouble reproducing now.  Did something change?
I don't see this either.
Seems like this was fixed around 2006-6-7.
Going to mark worksforme since it seems to not be a problem any more.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → WONTFIX
Hmm, well, I think I'm still seeing this occasionally, but on windows it just seems to hide the window and you can't raise it again.
I'll try to come up with a testcase, when I'm seeing that again.
Resolution: WONTFIX → WORKSFORME
I found bug 401946 that's maybe related to this bug (although that one seems to have a regression range that's more recent than this bug).
(In reply to comment #34)
> I found bug 401946 that's maybe related to this bug (although that one seems to
> have a regression range that's more recent than this bug).

does this imply bug 311223 is gone, or dependent on bug 401946? 

I don't think bug 401946 has anything to do with bug 311223.
Attached file testcase2
I see it occuring with this testcase, it usually happens within a few reloads. Like I said, on windows it just seems to hide the window and you can't raise it again.
Hmm, ok, that one doesn't seem to show the bug online, so you need to download that testcase to your computer.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
The testcase is changing the display of the <window> to 'table', then reloading the document. What's happening here is that nsIDocument::SetParentDocument(nsnull) is called on the old page when it is about to reload. However, due to the requested style change, a reflow of some sort is occuring on the old document afterwards.

The check in nsContainerFrame::SyncFrameViewGeometryDependantProperties which sets transparency on windows uses the nsIDocument::GetParentDocument as a means of distinguishing toplevel windows. Since the parent was cleared, the code thinks it is a toplevel window, and drops down and makes the window transparent.

What really needs to happen is either: not have the reflow occur once the parent is cleared, or make the check for toplevel windows more robust.

Note that the 'display: table' change isn't the only cause, any style change that causes a reflow and makes the root node transparent will have this effect.
 
Hmm.  What's the stack to that reflow that comes after SetParentDocument(nsnull)?  We don't do very much stuff between that call and tearing down the presshell, so I sort of wonder what in there triggers this.
Stack:

#0  SyncFrameViewGeometryDependentProperties (aPresContext=<value temporarily unavailable, due to optimizations>, aFrame=0xc298a0, aStyleContext=<value temporarily unavailable, due to optimizations>, aView=0x1ded29b0, aFlags=0) at /builds/trunk-project/mozilla/layout/generic/nsContainerFrame.cpp:479
#1  0x121c6287 in nsContainerFrame::SyncFrameViewAfterReflow (aPresContext=0x1ded20a0, aFrame=0xc298a0, aView=0xc22000, aCombinedArea=0xbfffc8b8, aFlags=1) at /builds/trunk-project/mozilla/layout/generic/nsContainerFrame.cpp:479
#2  0x12182227 in PresShell::DoReflow (this=0xb86000, target=0xc298a0) at /builds/trunk-project/mozilla/layout/generic/nsContainerFrame.cpp:479
#3  0x1218b52c in PresShell::ProcessReflowCommands (this=0xb86000, aInterruptible=1) at /builds/trunk-project/mozilla/layout/generic/nsContainerFrame.cpp:479
#4  0x1218e9be in PresShell::DoFlushPendingNotifications (this=0xb86000, aType=Flush_Layout, aInterruptibleReflow=1) at /builds/trunk-project/mozilla/layout/generic/nsContainerFrame.cpp:479
#5  0x1218eaae in PresShell::WillPaint (this=0xb86000) at /builds/trunk-project/mozilla/layout/generic/nsContainerFrame.cpp:479
#6  0x126815aa in nsViewManager::DispatchEvent (this=0x1ded2950, aEvent=0xbfffcc2c, aStatus=0xbfffcb1c) at /builds/trunk-project/mozilla/layout/generic/nsContainerFrame.cpp:479
#7  0x12676f74 in HandleEvent (aEvent=0xbfffcc2c) at /builds/trunk-project/mozilla/layout/generic/nsContainerFrame.cpp:479
What's higher up the stack?  Are we just spinning the event loop at this point or something?  Or is something doing a sync invalidate?

I'd expect the base_win->Destroy() call in nsFrameLoader::Destroy to tear down the presshell and viewmanager, so I assume that that stack is happening between the SetSubDocumentFor() call there and the Destroy().  Is that correct?  If so, I'd like to know where in there it happens.  If it's not correct, then we need to figure out why base_win->Destroy() doesn't tear down stuff.
(In reply to comment #42)
> What's higher up the stack?  Are we just spinning the event loop at this point
> or something?  Or is something doing a sync invalidate?
> 

I posted a Mac stacktrace. Above that point in the stack is Mac specific code running from a OS request to repaint. I can't generate a stacktrace on Windows currently, but it appears to be from a different point, running from within PresShell::ReflowEvent::Run.

> I'd expect the base_win->Destroy() call in nsFrameLoader::Destroy

nsFrameLoader::Destroy is never called. Should it be called during a reload?

> nsFrameLoader::Destroy is never called

Then what's callig SetParentDocument(nsnull)?
This is the stack where SetSubDocumentFor is called:

#0  DocumentViewerImpl::SyncParentSubDocMap (this=0x1dde3dd0) at /builds/trunk-project/mozilla/layout/base/nsDocumentViewer.cpp:615
#1  0x121598c2 in DocumentViewerImpl::SetContainer (this=0x1dde3dd0, aContainer=0x1dd3b0b4) at /builds/trunk-project/mozilla/layout/base/nsDocumentViewer.cpp:635
#2  0x16ec0b87 in nsDocShell::NewContentViewerObj (this=0x1dd3b000, aContentType=0x1dd28dc8 "application/vnd.mozilla.xul+xml", request=0x12a7ba48, aLoadGroup=0x1dd3c3d0, aContentHandler=0x1de21780, aViewer=0xbfffdb70) at /builds/trunk-project/mozilla/docshell/base/nsDocShell.cpp:6068
#3  0x16ed77e5 in nsDocShell::CreateContentViewer (this=0x1dd3b000, aContentType=0x12a7ba48 "Zqe\022\344ee\0222fe\022\b\273:\022\020\273:\022T\"?\022\200\367>\022\212\367>\022\254\277?\022\004\275?\022\232\231?\022<\276?\022\352\306e\022\336\264e\022Z\353>\022\024\376>\022l\353>\022\214\353>\022\306\354>\022^\243?\022\016\207B\0220WC\022r\206B\022\310\205B\022\016\356>\022,\356>\022\372\213Y\022&7B\022\030\273:\022\246\361e\022B8?\022\334D@\022\\\351>\022\024k@\022\300\351>\022\332\351>\022\272K?\022R4?\022\362\351>\022Fne\022rne\022~\177@\0228|?\022\034\003@\022d.?\0222\020?\022\226\016?\022t\352>\022\314\352>\022\266\006?\022"..., request=0x1dd28d50, aContentHandler=0x1de21780) at /builds/trunk-project/mozilla/docshell/base/nsDocShell.cpp:5916
#4  0x16ef2f1c in nsDSURIContentListener::DoContent (this=0x1dd3f4f0, aContentType=0x1dd28dc8 "application/vnd.mozilla.xul+xml", aIsContentPreferred=0, request=0x1dd28d50, aContentHandler=0x1de21780, aAbortProcess=0xbfffdc54) at /builds/trunk-project/mozilla/docshell/base/nsDSURIContentListener.cpp:138
#5  0x16efcf6f in nsDocumentOpenInfo::TryContentListener (this=0x1de21770, aListener=0x1dd3f4f0, aChannel=0x1dd28d50) at /builds/trunk-project/mozilla/uriloader/base/nsURILoader.cpp:735
#6  0x16efdc52 in nsDocumentOpenInfo::DispatchContent (this=0x1de21770, request=0x1dd28d50, aCtxt=0x0) at /builds/trunk-project/mozilla/uriloader/base/nsURILoader.cpp:434
#7  0x16efee99 in nsDocumentOpenInfo::OnStartRequest (this=0x1de21770, request=0x1dd28d50, aCtxt=0x0) at /builds/trunk-project/mozilla/uriloader/base/nsURILoader.cpp:280
#8  0x11006374 in nsBaseChannel::OnStartRequest (this=0x1dd28d20, request=0x1ddd7c80, ctxt=0x0) at /builds/trunk-project/mozilla/netwerk/base/src/nsBaseChannel.cpp:604
#9  0x110145ce in nsInputStreamPump::OnStateStart (this=0x1ddd7c80) at /builds/trunk-project/mozilla/netwerk/base/src/nsInputStreamPump.cpp:439
#10 0x11014eeb in nsInputStreamPump::OnInputStreamReady (this=0x1ddd7c80, stream=0x1e616a4c) at /builds/trunk-project/mozilla/netwerk/base/src/nsInputStreamPump.cpp:395
#11 0x0025c766 in nsInputStreamReadyEvent::Run (this=0x1ddd7d40) at /builds/trunk-project/mozilla/xpcom/io/nsStreamUtils.cpp:111
#12 0x0028964e in nsThread::ProcessNextEvent (this=0x1ddd7d50, mayWait=0, result=0xbfffe14c) at /builds/trunk-project/mozilla/xpcom/threads/nsThread.cpp:490
Hmm... so we're freezing the old document (the new one is still in paint suppression).

I guess the problem is that there's no way to detect that case (in particular, someone can call setAttribute() on a node in an already-unloaded document).  I guess we could look at the document container instead or something...
(In reply to comment #46)
> Hmm... so we're freezing the old document (the new one is still in paint
> suppression).
> 
> I guess the problem is that there's no way to detect that case (in particular,
> someone can call setAttribute() on a node in an already-unloaded document).  I
> guess we could look at the document container instead or something...
> 

I'm not really sure what this means.
> I'm not really sure what this means.

We're reflowing a document after it's not longer the "current" document in its docshell in some senses... but is in others.  The whole paint suppression mess.

Perhaps we should simply add a call to nsPresContext::IsChrome() here?  That would at least restrict the issue to chrome prescontexts...  Ideally, when we sync the parent subdoc map we'd somehow flag the "old" document as no longer allowing reflow, of course.
Blocks: 409223
The document's container is still the correct docshell, right? Instead of checking whether the document has a parent, we could check whether its docshell's mainWidget's nsWindowType is a toplevel/dialog/popup.
> The document's container is still the correct docshell, right?

It should be, yes.  At least until we Hide() the content viewer, after which we shouldn't have a presshell.  So yeah, using the docshell could work.  Heck, we could even check whether the docshell is the root docshell, instead of looking at its widget.
I'm thinking of the embedding case. We might have a root docshell that isn't actually a root widget.
Assignee: enndeakin → roc
Status: REOPENED → NEW
We could also check for chrome type, but maybe just checking the widget type is better.
Attached patch fix?Splinter Review
I can't reproduce this bug on Mac or Windows. Martijn, can you check if this fixes it for you?
Yes, that patch seems to fix it, at least for testcase2.
Assignee: roc → nobody
Component: XUL Widgets → Layout
Product: Toolkit → Core
QA Contact: xul.widgets → layout
Comment on attachment 295893 [details] [diff] [review]
fix?

Okay then, this should be good.
Attachment #295893 - Flags: superreview?(bzbarsky)
Attachment #295893 - Flags: review?(bzbarsky)
Comment on attachment 295893 [details] [diff] [review]
fix?

For good measure, do we want to also check that this is a chrome prescontext?  Or do we not care?  I can't think of cases offhand where a non-chrome one would pass this test...
Attachment #295893 - Flags: superreview?(bzbarsky)
Attachment #295893 - Flags: superreview+
Attachment #295893 - Flags: review?(bzbarsky)
Attachment #295893 - Flags: review+
checked in
Status: NEW → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008011005 Minefield/3.0b3pre
Status: RESOLVED → VERIFIED
Blocks: 379256
You need to log in before you can comment on or make changes to this bug.