59497 - A blank window opens when clicking on the Browse button the 2nd time

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[Srinivasa Rao Borusu]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.0; Windows 95; DigExt; by 
IndiaTimes)
BuildID:    20001010114

Unable to browse a page second time using browse button in composer. We are 
getting empty page.

Reproducible: Always

Steps to Reproduce:
1. Open existing document in new composer window.
2. Click on Browse button on the composition toolbar.
3. The page will be open in browser.
4. Change to previous composer window, click again on Browse button.
5. Then U will get a empty page in previous browser window.

Actual Results: Empty page in browser window.

Expected Results: It should open the corresponding page in browser.

Comment 1

[sujay]

I can't reproduce this on Win 98....it worksforme...

Comment 2

[Mike Kaply [:mkaply]]

Actually, in the current nightly, this crashes.

Try this:

Open just a composer window (no browser)

Open a local html file in composer.

Click on Browse button.

Switch back to composer window.

Click Browse button again.

Crash.

Comment 3

[rubydoo123]

on win95 using the release version, this doesn't crash, nor do I get the blank
window -- what I do get is a new browser window. Today's trunk builds are
horked, will test this again using tomorrows build

Comment 4

[Michael Ward]

Confirm crash with todays build on Win98 SE

Comment 5

[kinmoz]

Attachment: Stack trace for crash when browsing 2nd time.

Comment 6

[kinmoz]

I just attatched a stack trace that shows we are crashing in
nsDSURIContentListener::OnStartURIOpen(). I believe [e-mail] owns this.

Comment 7

[Adam Lock]

This was a tricky one to pin down but I'll go over the what I think is
happening:

1. nsIURIContentListeners are hierarchical in nature, and
   calls such as onStartURIOpen are passed upwards through any
   parent listener registered via the parentContentListener attribute.
2. Docshell is crashing in its nsDSURIContentListener::OnStartURIOpen()
   because the registered parentContentListener isn't there any
   more. The pointer points to garbage invalid.
3. The parent content listener that was registered was implemented by
   an nsBrowserInstance object.
4. Navigator.js created this browserinstance object during initialisation
   (the "appCore") and placing a reference to it in its _content docshell's
   script context.
5. The _content area gets replaced by the second click on the Editor's "Browse"
   button so that appCore object gets destroyed (via garbage collection).
6. The crash happens when the new content arrives.

Unless it is possible to move appCore out of navigator's _content context and up
into the chrome I think the best solution to this issue is to modify the way the 
editor opens this window in the first place. Instead of using openDialog, it 
should probably find the named window and control it much the same way as 
navigator does.

CC'ing Simon

Comment 8

[Adam Lock]

Attachment: Patch to fix this crasher

Comment 9

[Adam Lock]

To reiterate was was causing the crash -

When window.openDialog is passed the name of a window that already exists it 
rips out the content docshell and loads in the new one. Navigator stores its 
appCore variable in the content docshell so that gets destroyed as well. 
The docshell still has the appCore registered as a parent listener so you get 
a crash when it is next called (during the next uri load). Hence the reason why 
the preview panel crashes on the second click.

The patch fixes the preview function by first searching for the named window 
and only calling window.openDialog it if isn't found.

Simon can you review this patch please and comment on the ramifications of this 
limitation with openDialog? I notice the same method being used extensively 
elsewhere in the editor.

Thanks 

Comment 10

[Simon Fraser [no longer active]]

cc charley

Comment 11

[Charles Manske]

Your fix certainly makes sense, and this is the only case where we call
openDialog with a named window. All other calls use "_blank", so we get a new
window.
Bug this seems to be a bad limitation -- shouldn't we put your suggested logic
into the openDialog code instead of just in editor?

Comment 12

[Simon Fraser [no longer active]]

I think we have to fix this in C++, not hack around it with JS changes. It seems 
to be an XPToolkit bug.

Comment 13

[Adam Lock]

The proper solution may be for openDialog to assert and return with an error if 
the named window already exists. Otherwise all sorts of dangerous things will 
continue to happen when a window's docshell is simply replaced without notifying 
it. In the few cases where this impacts on existing code we should make it 
search for the named window first with the windows mediator or some other means.

Comment 14

[Alec Flett]

yeah, window.openDialog should assert, and then either throw an exception or do
absolutely nothing...

Comment 15

[anthonyd]

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

Comment 16

[anthonyd]

adding shaver to the cc list.

anthonyd

Comment 17

[Judson Valeski]

nom. moz0.9.

note: I can repro this on win2k w/ build from yesterday.

Comment 18

[Dan M]

  I just noticed this bug (again). Adam, Alec -- Maybe I don't quite follow, but 
your above proposed solution seems wrong to me. You give a window a name so you 
can rig subsequent opens to show up in that window if it still exists, or make a 
new one if not. That's all the window.open |name| parameter is useful for. Well, 
besides special targets like _top. That is what you're talking about, isn't it?
  Sounds like the real problem is that some things are being cached and not 
cleaned up when necessary.

Comment 19

[Adam Lock]

When the openDialog method rips out part of a window it is asking for trouble. 
In this case, the navigator chrome stored some stuff in the old docshell and 
never had the chance to clean it up properly.

I just think it is dangerous that the openDialog method can do this. The second 
patch demonstrates that it's not hard to make the code work if openDialog 
returns an error instead.

Comment 20

[Brendan Eich [:brendan]]

Adam: if there is a dangling pointer in some docshell-related data structure,
that's a bug in the docshell, at least (possibly in other code too).  We have
strong XPCOM refs.  We have safe nsWeakPtrs.  Why should openDialog or anything
else worry about this docshell bug?  Why shouldn't the docshell be fixed to
clean up, or cause browser app code to clean up, the bad pointer?

/be

Comment 21

[Adam Lock]

Brendan, the bug is either in navigator for installing a content listener in 
the script context of one of its child docshells, or it's in openDialog for 
brutally replacing the middle of a navigator window and not even telling it.

It has nothing to do with docshell. Docshell just happened to be the one holding 
the bad pointer, but it could happen with any kind of object that someone holds 
a pointer when it is destroyed by openDialog.

Comment 22

[Dan M]

  A notification would be fair. Or perhaps some less brutal treatment from 
OpenDialog, if you have something in mind. But the open operation locking onto an 
extant window of the same name; all browsers do that.

Comment 23

[Brendan Eich [:brendan]]

Adam: unconfuse me please: either the docshell holds onto content listeners
using strong refs, in which case you're saying someone clobbers a referenced
object and frees its memory?  Or docshell holds a weak pointer, in which case I
still say it should share some blame, and use an nsWeakPtr or some such?

/be

Comment 24

[Adam Lock]

Docshell isn't at fault here. Yes proper weak references might make it more 
robust, but it would be masking the problem in openDialog.

I believe that we should change the few places that rely on this code to use the 
mediator instead. See the patch

Comment 25

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

So here's what I don't understand, if any of you care to explain, is why the
appCore (spit) doesn't just unregister itself when it's destroyed.  (And why is
window.openDialog any different from window.open?  It looks like it calls the
same code, to my untrained eye.)

Comment 26

[Simon Fraser [no longer active]]

Responding to Adam Lock's last comment: if the call sites are changed, that does 
not address the real issue (as we have noted above in this bug). A XUL writer 
would still be able to write XUL/JS that causes this crash, and that should not 
be possible. Bad XUL may cause unpredictable behaviour, but it should never 
crash. I still think we need to address the root cause.

Comment 27

[Adam Lock]

This is the offending piece of code in navigator.js

http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator
.js#470

I suspect this bug will go away if appCore doesn't need to be stored in 
_content.

CC'ing the blame author for comment.

Comment 28

[Peter "jag" Annema]

Adam Lock: that line can indeed be removed (it was used by directory.xul/js a
long time ago).

But that's just avoiding this crash. Why is window.openDialog ripping out the
docShell and replacing it with a new one? Why not just tell the existing
docShell to load the new URI?

It would also be nice if there'd be a mechanism for interested parties to find
out when a docshell changes. E.g. I have a WebProgressListener hooked up to
_content's docShell which I need to hook up again when _content's docShell
changes.

Comment 29

[Adam Lock]

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

Comment 30

[Adam Lock]

I've just step debugged this again and the _content.appCore thing seems to be a 
red herring. openDialog actually works on the topmost docshell and not the 
content docshell as I previously thought. 

The issue appears to be that Navigator does all its startup and shutdown stuff 
in the window onload and onunload handlers. This includes the registering / 
deregistering of the web progress listener.

openDialog doesn't fire onload or onunload events to the window so the old 
navigator isn't shutdown and the new navigator isn't started up correctly. We're 
left with a dangling pointer from the old navigator which crashes mozilla as the 
new navigator is loaded because unregistration code was never called.

I think this proves that openDialog is fundamentally broken. A lot of our chrome 
uses window opening and closing events to setup and shutdown. openDialog will 
not call these routines when loading content into existing named docshells.

Comment 31

[sairuh (rarely reading bugmail)]

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

Comment 32

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

As far as I can tell, it's not openDialog that's destroying the docshell; it's
something else that's releasing the last reference. openDialog doesn't, and
shouldn't, know about the ownership model of the docshells here.

If there's a need for onUnload to be run when new content is loaded into a
docshell, or when a docshell is destroyed, shouldn't the docshell be doing that,
perhaps from its destructor?  It seems pretty broken for us to expect all the
all sites to check -- and care -- whether or not there's an existing docshell by
that name, and whether or not it has content loaded, and then fire the event
handlers, etc.

Navigator 2.x managed to file onUnload handlers when content script or a
targetted link replaced the content of an existing window, without any special
behaviour on the part of the content author.  I'm pretty sure we handle that
case as well.  Why can't the same mechanism apply here?

Comment 33

[Adam Lock]

The docshell isn't being destroyed. It's being told (via LoadURI) to load new 
chrome content. The old chrome is being swapped out and in the case of 
navigator.xul this means its Shutdown() is never called because no window 
onunload event fires before it happens.

navigator.xul also uses the window onload event to do it's startup so the new 
chrome content wouldn't initialise correctly either. However the dangling 
pointer from the old chrome means it crashes before it even gets that far. If it 
did get past the pointer crash you would see lots of Javascript errors 
complaining that the objects meant to be created during startup didn't exist.

Comment 34

[Peter "jag" Annema]

So that suggests onload and onunload shouldn't be called when the window is
loaded / unloaded, but rather when the (xul) window's docshell is
created/destroyed, right?

Comment 35

[Peter "jag" Annema]

Erh, not docshell, but whatever it is that represents the xul file being
loaded/replaced.

Comment 36

[Dan M]

This patch fixes the crash. Use it in good health. You'll notice the browser 
window, now all uncrashing, is now forgetting to load the content URL. Bummer. 
That'd be another bug. I blame the BrowserInstance, but it's probably just habit.

Index: nsBrowserInstance.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/browser/src/nsBrowserInstance.cpp,v
retrieving revision 1.193
diff -u -r1.193 nsBrowserInstance.cpp
--- nsBrowserInstance.cpp	2001/03/25 16:49:18	1.193
+++ nsBrowserInstance.cpp	2001/03/30 05:28:59
@@ -792,6 +792,15 @@
   if (NS_SUCCEEDED(rv)) 
     rv = dispatcher->UnRegisterContentListener(this);
 
+  // we are no longer the chrome docshell's content listener, either
+  nsCOMPtr<nsIScriptGlobalObject> globalObj(do_QueryInterface(mDOMWindow));
+  if (globalObj) {
+    nsCOMPtr<nsIDocShell> docShell;
+    globalObj->GetDocShell(getter_AddRefs(docShell));
+    if (docShell)
+      docShell->SetParentURIContentListener(0);
+  }
+
   return NS_OK;
 }
 

Comment 37

[Adam Lock]

Dan, the problem isn't with content listener, it's with openDialog. If the 
window onunload had fired before the new chrome had loaded, the old chrome would 
have removed the listener properly. The additional problems you mention are 
because the new chrome doesn't get a window onload so it hasn't set itself up 
properly.

A custory search through LXR shows we use named windows for most of the 
singleton windows. Any one of these is a potential crasher because the chrome's 
initialisation and shutdown routines may not be called correctly:

http://lxr.mozilla.org/seamonkey/source/editor/ui/composer/content/sb-bookmarks.
js#51
http://lxr.mozilla.org/seamonkey/source/editor/ui/composer/content/sb-file-panel
.js#47
http://lxr.mozilla.org/seamonkey/source/extensions/wallet/resources/content/wall
etOverlay.js#114
http://lxr.mozilla.org/seamonkey/source/extensions/wallet/resources/content/wall
etTasksOverlay.xul#134
http://lxr.mozilla.org/seamonkey/source/extensions/wallet/resources/content/wall
etTasksOverlay.xul#141
http://lxr.mozilla.org/seamonkey/source/extensions/xmlterm/ui/content/XMLTermCom
mands.js#214
http://lxr.mozilla.org/seamonkey/source/extensions/inspector/resources/content/I
nspectorApp.js#267
http://lxr.mozilla.org/seamonkey/source/extensions/inspector/resources/content/s﷒0﷓
http://lxr.mozilla.org/seamonkey/source/extensions/inspector/resources/content/s﷒1﷓
http://lxr.mozilla.org/seamonkey/source/extensions/p3p/resources/content/pref-P3
P.js#394
http://lxr.mozilla.org/seamonkey/source/profile/resources/content/profileManager
.js#37
http://lxr.mozilla.org/seamonkey/source/profile/resources/content/profileManager
.js#141
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator
.js#780
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator
Overlay.xul#272
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/book
marksOverlay.js#619
http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/con﷒2﷓
http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/con﷒3﷓
http://lxr.mozilla.org/seamonkey/source/xpfe/components/search/resources/search-
panel.js#729
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/charsetOve
rlay.xul#106
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/charsetOve
rlay.xul#204
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/charsetOve
rlay.xul#318
http://lxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/mailWind
owOverlay.js#815
http://lxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/mailWind
owOverlay.js#821
http://lxr.mozilla.org/seamonkey/source/mailnews/base/search/resources/content/F
ilterListDialog.js#156
http://lxr.mozilla.org/seamonkey/source/mailnews/base/search/resources/content/F
ilterListDialog.js#167

Due to the number of places this occurs (and possibly more in the commercial 
tree) I think fake window closure and window opening events to the outgoing and 
incoming chrome could be the best option. The alternative is to change 
openDialog to error when the named window already exists and modify the JS to 
use the window mediator. The latter is a lot more work.

What are the issues with faking these events? Is it feasible?

This patch below changes openDialog to throw an error when a named docshell 
already exists. The second attachment shows the kind of code we would have to 
apply if we used windows mediators.

Index: mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp
===================================================================
RCS file: 
/cvsroot/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp,v
retrieving revision 1.14
diff -u -r1.14 nsWindowWatcher.cpp
--- nsWindowWatcher.cpp	2001/03/24 00:52:16	1.14
+++ nsWindowWatcher.cpp	2001/03/30 11:21:19
@@ -538,6 +538,16 @@
         newTreeOwner->SetPersistence(PR_FALSE, PR_FALSE, PR_FALSE);
     }
   }
+  else {
+    /* Named chrome docshells cannot be reused in case they rely on windows
+       events eg load, unload to cleanup correctly. */
+    PRInt32 type;
+    newDocShellItem->GetItemType(&type);
+    if (type == nsIDocShellTreeItem::typeChrome) {
+      NS_ASSERTION(0, "Chrome docshells cannot be reused");
+      return NS_ERROR_FAILURE;
+    }
+  }
 
   if (aDialog && argc > 0)
     AttachArguments(*_retval, argc, argv);

Comment 38

[Dan M]

  Adam, you really do want to turn off the ability to open into an existing 
window. So restricting only chrome URLs would arguably satisfy the "DOM 
compatibility requirement," which knows nothing about chrome. But I think it's a 
logical extension to be able to expect chrome to behave the same as content. Your 
lxr list above is a list of some/all of the places that changing this assumption 
would break our code --  I'm assuming the authors of those lines used named 
windows because they wanted named window behaviour.
  In fact, I consider your lxr list an affirmation that reloading chrome URLs 
generally works. See all the places it's done, and yet we've noticed only this 
one problem. I'm convinced more than ever that this bug is one (or more) issues 
with the perennially misbehaved nsBrowserInstance.

  Really, this bug can (sort of) be fixed entirely with just this patch

Index: editor/ui/composer/content/ComposerCommands.js
===================================================================
RCS file: /cvsroot/mozilla/editor/ui/composer/content/ComposerCommands.js,v
retrieving revision 1.49
diff -u -r1.49 ComposerCommands.js
--- ComposerCommands.js	2001/03/28 21:16:41	1.49
+++ ComposerCommands.js	2001/03/30 19:43:14
@@ -499,7 +499,7 @@
 
     // Check if we saved again just in case?
 	  if (DocumentHasBeenSaved())
-	    window.openDialog(getBrowserURL(), "EditorPreview", "chrome,all,dialog=
no", window._content.location);
+	    window.open(window._content.location, "EditorPreview");
   }
 };
 
My version is theoretically equivalent, but hides the issue by working 
nsBrowserInstance into less of a froth. (Unfortunately, my version *should* work 
equivalently, but it points out a couple of other bugs in window opening. Both of 
them relatively minor, and wanting to be fixed, anyway.)

I can't condone disabling opening into an extant window. I say we check in the 
above change, fixes for the problems that it exposes, and while we're here, my 
patch from yesterday to nsBrowserInstance, no longer strictly necessary but it 
makes it clean up after itself a bit, which seems good.

Comment 39

[Adam Lock]

Dan, I don't know what the right solution is. I first thought that openDialog
should just fail for existing named windows, but given the number of places that
named windows are used, then perhaps it would be best to investigate posting
onload, onunload window events at the right times instead.

Yes, there is only 1 known crash so far, but how many more places are failing in
subtle ways? Most .XUL files have onload handlers and if that's not being called
then who knows what might be going wrong?

We could certainly make appCore more tolerant of not being uninitialised but
that isn't fixing the underlying problem.

Comment 40

[R.K.Aa.]

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

Comment 41

[Adam Lock]

CC'ing Rick. Rick, does any of your window targettting work touch on this 
problem?

Comment 42

[Adam Lock]

This bug seems to have lessened in severity as it doesn't crash anymore but 
there is still problems.

I put some dump() statements into the navigator's JS Startup() and Shutdown() 
routines to ensure each is being called correctly when the same named 
"EditorPreview" window is used and then used for a second time. 

The routines appear to be called okay, but on the second time the Startup() 
routine has problems loading the URL specified in window.arguments[0]. I put 
another dump() statement to see what the value of that was and got:

chrome://navigator/content/navigator.js line 221: window.arguments has no 
properties

So window.arguments doesn't exist even though it should. Then I got an 
exception further along:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "gBrowser has no properties" 
{file:"chrome://communicator/content/securityUI.js" line: 35}]' when calling 
method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 
(NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"
data: yes]
************************************************************

It may be that the variables in the window's JS global context are wiped out 
when the first URL is destroyed and not initialised correctly for the second.

One interesting thing is where nsWindowWatcher::OpenWindowJS calls 
AttachArguments() to put the arguments[0] into the script context. It's being 
called before LoadURI, so possibly it doesn't survive when the previous URI is 
torn down. This wouldn't explain why I would see an exception referring to 
gBrowser however.

I will do some further investigation further tomorrow.

Comment 43

[Adam Lock]

Retargetting.

CC'ing jst & hyatt for ideas on why global variables are not in the script
context the second time around.

Comment 44

[Simon Fraser [no longer active]]

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

Comment 45

[rubydoo123]

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

Comment 46

[Katsuhiko Momoi]

** Observed with 7/23/2001 Win32 branch build **

With the above build, you now see a blank window if you try to view
the page you're composing by pressing on the Browse button the 2nd time.
I modified the summary line to fit the current problem.

This means that if you are preparing a Composer document and if you save
from time to time and want to see what the saved page looks like in 
a Browser window, the content does not get displayed but rather 
just a blank content.

I cannot call Composer with this kind of defect a functioning
Composer. Has any progress been made on this problem?

Comment 47

[selmer (gone)]

Kat, your description sounds suspiciously like 82720.  A fix went in for it, but
it doesn't look complete.  Can't tell if many people are hitting the remainder
or not, but this may be another manifestation of it.  If you believe that to be
so, please update 82720 also.

Comment 48

[Katsuhiko Momoi]

I tried to reproduce Bug 82720 and could not with 7/23/2001 Win32
branch build. I cannot tell if the problem described here reduces to 
that one. For one thing, this is creating a browser window with 
the content from the Composer window. The other one is about
clicking on a link to go to a page. 
I am more ocncerned about the impact to the user this bug produces.

Comment 49

[Chris Petersen]

I'm able to reproduce this same problem with the branch Aug 9th Windows and Mac
builds.
In fact, the Mac build crashes when clicking on the menu bar if the blank
browser window is the top most window.

Comment 50

[Chris Petersen]

Crashing occurs in both Mac OS 9 and Mac OS X builds:

Stack trace from Mac OS X's crash reporter

**********

Date/Time: 2001-08-09 14:47:08 -0700

PID:       249
Command:   Netscape 6

Exception: EXC_BAD_ACCESS (0x0001)
Codes:     KERN_PROTECTION_FAILURE (0x0002) at 0x00000000

Thread 0:
 #0   0x01effbd0 in 0x1effbd0 ()
 #1   0x01effbbc in 0x1effbbc ()
 #2   0x01efdc90 in 0x1efdc90 ()
 #3   0x01efe630 in 0x1efe630 ()
 #4   0x737e1350 in _InvokeEventHandlerUPP ()
 #5   0x737e1090 in _DispatchEventToHandlers ()
 #6   0x737e0d34 in _SendEventToEventTargetInternal ()
 #7   0x737e17a4 in _SendEventToEventTargetWithOptions ()
 #8   0x73a5a98c in _SendMenuOpening__FP8MenuDatadUlPUc ()
 #9   0x73929118 in _DrawTheMenu__FP14MenuSelectDataPP9__CFArray ()
 #10  0x739ff0e0 in _MenuChanged__FP14MenuSelectData ()
 #11  0x73ab6fd8 in _TrackMenuCommon__FR14MenuSelectDataPUc ()
 #12  0x739ff768 in _MenuSelectCore__FG5PointdUlPP16OpaqueMenuHandlePUs ()
 #13  0x739ff65c in _MenuSelect ()
 #14  0x01ee88f4 in 0x1ee88f4 ()
 #15  0x01ee865c in 0x1ee865c ()
 #16  0x01ee81c4 in 0x1ee81c4 ()
 #17  0x01ee7cfc in 0x1ee7cfc ()
 #18  0x01ceb14c in 0x1ceb14c ()
 #19  0x000939ac in 0x939ac ()
 #20  0x0009434c in 0x9434c ()

Thread 1:
 #0   0x7000424c in _syscall ()
 #1   0x706584b8 in _ProcessReadyEvent ()
 #2   0x706582b0 in _CarbonSelectThreadFunc ()
 #3   0x70014f04 in __pthread_body ()

Thread 2:
 #0   0x70059b68 in _semaphore_wait_signal_trap ()
 #1   0x70016110 in _semaphore_wait_signal ()
 #2   0x70015f78 in __pthread_cond_wait ()
 #3   0x70015d18 in _pthread_cond_wait ()
 #4   0x70653be0 in _BSD_pthread_cond_wait ()
 #5   0x70653bc0 in _CarbonConditionWait ()
 #6   0x7065557c in _CarbonOperationThreadFunc ()
 #7   0x70014f04 in __pthread_body ()

Thread 3:
 #0   0x70059b48 in _semaphore_timedwait_signal_trap ()
 #1   0x7003f7f8 in _semaphore_timedwait_signal ()
 #2   0x70015f68 in __pthread_cond_wait ()
 #3   0x7003f7c4 in _pthread_cond_timedwait_relative_np ()
 #4   0x7029b590 in _TSWaitOnConditionTimedRelative ()
 #5   0x7029cdac in _TSWaitOnSemaphoreCommon ()
 #6   0x702e5f98 in _TSWaitOnSemaphoreRelative ()
 #7   0x702e7208 in _TimerThread ()
 #8   0x70014f04 in __pthread_body ()

Thread 4:
 #0   0x70059b68 in _semaphore_wait_signal_trap ()
 #1   0x70016110 in _semaphore_wait_signal ()
 #2   0x70015f78 in __pthread_cond_wait ()
 #3   0x70015d18 in _pthread_cond_wait ()
 #4   0x7029b550 in _TSWaitOnCondition ()
 #5   0x7029cd94 in _TSWaitOnSemaphoreCommon ()
 #6   0x7029cce4 in _TSWaitOnSemaphore ()
 #7   0x7029cba8 in _AsyncFileThread ()
 #8   0x70014f04 in __pthread_body ()

Thread 5:
 #0   0x70059b68 in _semaphore_wait_signal_trap ()
 #1   0x70016110 in _semaphore_wait_signal ()
 #2   0x70015f78 in __pthread_cond_wait ()
 #3   0x70015d18 in _pthread_cond_wait ()
 #4   0x70653be0 in _BSD_pthread_cond_wait ()
 #5   0x70653bc0 in _CarbonConditionWait ()
 #6   0x70653ab4 in _CarbonInetOperThreadFunc ()
 #7   0x70014f04 in __pthread_body ()

Thread 6:
 #0   0x70059b68 in _semaphore_wait_signal_trap ()
 #1   0x70016110 in _semaphore_wait_signal ()
 #2   0x70015f78 in __pthread_cond_wait ()
 #3   0x70015d18 in _pthread_cond_wait ()
 #4   0x7029b550 in _TSWaitOnCondition ()
 #5   0x7029b59c in _TSWaitOnConditionTimedRelative ()
 #6   0x7029b6a4 in _MPWaitOnQueue ()
 #7   0x708de050 in _SyncTaskProc__13TNodeSyncTaskPv ()
 #8   0x70291434 in _PrivateMPEntryPoint ()
 #9   0x70014f04 in __pthread_body ()

Thread 7:
 #0   0x700007b8 in _mach_msg_overwrite_trap ()
 #1   0x700056e4 in _mach_msg_overwrite ()
 #2   0x700277b0 in _thread_suspend ()
 #3   0x70027744 in __pthread_become_available ()
 #4   0x70027468 in _pthread_exit ()
 #5   0x70014f08 in __pthread_body ()

PPC Thread State:
  srr0: 0x01effbd0 srr1: 0x0000f030                vrsave: 0x00000004
   xer: 0x0000000c   lr: 0x01effbbc  ctr: 0x0013769c   mq: 0x00000000
    r0: 0x00000000   r1: 0xbfffe330   r2: 0x01f5d000   r3: 0x00000000
    r4: 0xbfffe830   r5: 0xbfffe82c   r6: 0x00000000   r7: 0xbfffe830
    r8: 0x00000000   r9: 0x0232160c  r10: 0x023274b0  r11: 0x0234a648
   r12: 0x002566c0  r13: 0x01f60154  r14: 0xbfffe450  r15: 0xbfffe82c
   r16: 0x01f5b658  r17: 0xbfffe40c  r18: 0xbfffe8cc  r19: 0xbfffe8c8
   r20: 0xbfffe408  r21: 0xbfffe404  r22: 0x01f5b1b0  r23: 0x01f5d0f4
   r24: 0x0026b220  r25: 0x01f60138  r26: 0x01f60130  r27: 0xbfffe8e0
   r28: 0x01f60134  r29: 0x0418a9c0  r30: 0x00000001  r31: 0x00000000

**********

Comment 51

[TucsonTester2]

One thing I noticed with this bug is that even if you use the browser window to
go to different sites this issue will still occur.  Don't know if this helps at all.

Comment 52

[Charles Manske]

We're still crashing? (Doesn't seem to in Windows)
Sure wish this was fixed for 0.9.4!

Comment 53

[Adam Lock]

Retargetting

Comment 54

[Charles Manske]

Adding status designations to keep our attention on this problem.

Comment 55

[Asa Dotzler [:asa]]

0.9.5 is out the door. bumping TM up by one.

Comment 56

[Adam Lock]

Retargetting.

If someone has a good idea to fix this bug please take it.

Comment 57

[sujay]

Marking nsbeta1+ per syd. Need to fix for composer usability.

Comment 58

[Charles Manske]

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

Comment 59

[TucsonTester1]

Works for me on mac os x build 2001121805, using both original steps and revised
steps as noted in Comment 2.

Comment 60

[Charles Manske]

I'm still seeing the problem in my debug build (12/17 codebase).
1. Edit a page.
2. Use "Browse" button on toolar
3. Save page when prompted. (Browser window displays with page)
4. Make a change to page in Composer.5. Use "Browse" button on toolar again and
answer "Yes" to saving page.
Browser window that had the page before will become empty.

Comment 61

[Syd Logan]

reproduced in Linux, marking +, adam, what's the chance of a fix by mozilla 1.0?

Comment 62

[Charles Manske]

Attachment: Fix in Composer code

This fix is in nsPreviewComand.doCommand()
It hides an underlying problem, but it a good fix because it finds the existing

browser window used for the first Preview, which we should have been doing
anyway.

Comment 63

[Charles Manske]

I'll take the bug for now.
Should we open a separate bug on the issue of not being able to reload a url
into a "named window"?
Realizing that that was behind this problem I think it is better to use "_blank"
when starting a new browser window for previewing. New patch comming.

Comment 64

[Charles Manske]

Attachment: Update on Composer fix.

Don't use named window for Preview browser.
This means a new preview window will be loaded for each Composer page being 
edited, but that's not bad since we find the preexisting preview browser
when reusing Preview button for a particular page.

Comment 65

[Kathleen :Brade]

Comment on attachment 72514 [details] [diff] [review]
Update on Composer fix.

This patch needs some work.  It seems to do a lot of needless checking like:
+      var windowManagerInterface =
windowManager.QueryInterface(Components.interfaces.nsIWindowMediator);
+      if ( !windowManagerInterface )
+	 return;

In reality, what will happen if the QI fails is that an error will be thrown. 
I don't see a try/catch around this block.  Is there one?

Comment 66

[Charles Manske]

Attachment: Update to Composer fix

Put all interface-related calls inside the try

Comment 67

[Kathleen :Brade]

Comment on attachment 72613 [details] [diff] [review]
Update to Composer fix

r=brade

Comment 68

[kinmoz]

Comment on attachment 72613 [details] [diff] [review]
Update to Composer fix

sr=[e-mail]

Can we fix the indentation of the |if| you are modifying?


 // Check if we saved again just in case?
	  if (DocumentHasBeenSaved())
-	    window.openDialog(getBrowserURL(), "EditorPreview",
"chrome,all,dialog=no", window._content.location);
+    {
+      var browser;


No need to use an |else| if the |if| causes us to exit the loop:


+	   if ( browser && (window._content.location.href ==
browser._content.location.href))
+	     break;
+	   else
+	     browser = null;


Is there any possibility of the window.openDialog() or setTimeout() calls
throwing an error?

Also, what are we going to do about the original problem in the dialog code?
This bug seems to contain a lot of important info.

Comment 69

[Charles Manske]

Fixed items mentioned by Kin.
After checking this in, I'll reassign bug to Adam Lock to address original 
observation about not being able to reuse a "named" window.

Comment 70

[Brendan Eich [:brendan]]

cmanske, how about removing the else after break and unindenting the next line
one stop?

+
   if ( browser && (window._content.location.href ==
browser._content.location.href))
+
     break;
+
   else
+
     browser = null;

/be

Comment 71

[Charles Manske]

Brendan. Yes, I did, as Kin suggested. I just didn't update the patch.
It is now:
  browser = windowManagerInterface.convertISupportsToDOMWindow(
enumerator.getNext() );
  if ( browser && (window._content.location.href == browser._content.location.href))
            break;

  browser = null;

Comment 72

[Asa Dotzler [:asa]]

Comment on attachment 72613 [details] [diff] [review]
Update to Composer fix

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 73

[Charles Manske]

checked in. 
After this is verified, I'll reopen or file a new bug for the "can't reuse a
named window" issue. There's a lot of useful discussion in this bug we don't 
want to loose!

Comment 74

[Dawn Endico]

removed the item for this bug from the release notes for 0.9.9 and beyond,
because the bug is fixed now. Let me know if you think the item should be 
re-added.

Comment 75

[sujay]

verified in 3/7 build.

marking Verified-fixed..if anyone is still having any problems, please
REOPEN.

this should be fixed for everyone who uses 3/7 build.