322074 - couple of xul tags hide window on unload

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Bernd]

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.

Comment 1

[Bernd]

Attachment: testcase

Comment 2

[Bernd]

loading the testcase and hitting back is also "efficient"

Comment 3

[:Gavin Sharp [email: [e-mail]]]

confirmed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051230 Firefox/1.6a1. Pretty efficient DoS.

Comment 4

[Bernd]

Attachment: shorter testcase

Comment 5

[Bernd]

we hit
###!!! ASSERTION: failed to get the nsIScriptGlobalObject. bug 95465?: 'boundGlobal', file d:/moz_src/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp, line 439

Comment 6

[Bernd]

Boris do you know who deals with that kind of assert?

Comment 7

[Ria Klaassen (not reading all bugmail)]

It has even a regression date: 1.9a1_2005083018 - 1.9a1_2005083107
(tried 3x reload, 1x back)

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-08-30+17%3A00%3A00&maxdate=2005-08-31+07%3A00%3A00&cvsroot=%2Fcvsroot

Comment 8

[Boris Zbarsky [:bzbarsky]]

Um... hyatt?  Or maybe bryner, jst, or me.  :(

Comment 9

[Bernd]

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

Comment 10

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #9)
Found one more build (1.9a1_2005083019) which shows the bug and now I see this:

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-08-30+17%3A00%3A00&maxdate=2005-08-30+19%3A00%3A00&cvsroot=%2Fcvsroot

Comment 11

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 12

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #11)
Not anymore, but also in branch it regressed. 
Between 1.8b4_2005083106 and 1.8b4_2005083120.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 14

[Ria Klaassen (not reading all bugmail)]

Something fixed it in branch between 1.8b5_2005092913 and 1.8b5_2005092922.

http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?treeid=default&module=AviarySuiteBranchTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-09-29+12%3A00%3A00&maxdate=2005-09-29+22%3A00%3A00&cvsroot=%2Fcvsroot

Comment 15

[Bernd]

Does the trunk also change at these days (1.8b5_2005092913 and 1.8b5_2005092922) the bevahior back?

Comment 16

[Ria Klaassen (not reading all bugmail)]

(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

Comment 17

[Bernd]

I would say there was a backout

fixed on branch
http://tinderbox.mozilla.org/bonsai/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/view/src&command=DIFF_FRAMESET&file=nsViewManager.cpp&rev1=3.411&rev2=3.411.2.1&root=/cvsroot

backout on trunk
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/view/src&command=DIFF_FRAMESET&file=nsViewManager.cpp&rev1=3.418&rev2=3.419&root=/cvsroot

Comment 18

[Bernd]

pointing to bug 301411 and bug 311223

Comment 19

[Bernd]

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[[e-mail]]

(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.

Comment 22

[[e-mail]]

Attachment: Lazy fix

Note: hwaara was also working improving things in bug 222274.

Comment 23

[[e-mail]]

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.

Comment 24

[[e-mail]]

It's not helping that the under/overflow events don't have a true event target.

Comment 25

[[e-mail]]

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.

Comment 26

[Håkan Waara]

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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

I can reproduce just fine with a current trunk (well, Friday night) debug Linux Firefox build (using "shorter testcase").

Comment 28

[Benjamin Smedberg]

Neil, can you take this?

Comment 29

[Boris Zbarsky [:bzbarsky]]

So I'm actually having trouble reproducing now.  Did something change?

Comment 30

[Neil Deakin]

I don't see this either.

Comment 31

[Neil Deakin]

Seems like this was fixed around 2006-6-7.

Comment 32

[Neil Deakin]

Going to mark worksforme since it seems to not be a problem any more.

Comment 33

[Martijn Wargers (dead)]

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.

Comment 34

[Martijn Wargers (dead)]

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).

Comment 35

[Wayne Mery (:wsmwk)]

(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? 

Comment 36

[Martijn Wargers (dead)]

I don't think bug 401946 has anything to do with bug 311223.

Comment 37

[Martijn Wargers (dead)]

Attachment: 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.

Comment 38

[Martijn Wargers (dead)]

Hmm, ok, that one doesn't seem to show the bug online, so you need to download that testcase to your computer.

Comment 39

[Neil Deakin]

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.
 

Comment 40

[Boris Zbarsky [:bzbarsky]]

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.

Comment 41

[Neil Deakin]

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

Comment 42

[Boris Zbarsky [:bzbarsky]]

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.

Comment 43

[Neil Deakin]

(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?

Comment 44

[Boris Zbarsky [:bzbarsky]]

> nsFrameLoader::Destroy is never called

Then what's callig SetParentDocument(nsnull)?

Comment 45

[Neil Deakin]

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

Comment 46

[Boris Zbarsky [:bzbarsky]]

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...

Comment 47

[Neil Deakin]

(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.

Comment 48

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 49

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

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.

Comment 50

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 51

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

I'm thinking of the embedding case. We might have a root docshell that isn't actually a root widget.

Comment 52

[Boris Zbarsky [:bzbarsky]]

We could also check for chrome type, but maybe just checking the widget type is better.

Comment 53

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

Attachment: fix?

I can't reproduce this bug on Mac or Windows. Martijn, can you check if this fixes it for you?

Comment 54

[Martijn Wargers (dead)]

Yes, that patch seems to fix it, at least for testcase2.

Comment 55

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

Comment on attachment 295893 [details] [diff] [review]
fix?

Okay then, this should be good.

Comment 56

[Boris Zbarsky [: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...

Comment 57

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

I don't think we care.

Comment 58

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

checked in

Comment 59

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008011005 Minefield/3.0b3pre