Closed Bug 59497 Opened 24 years ago Closed 23 years ago

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

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: sborusu, Assigned: cmanske)

References

Details

(Whiteboard: [QAHP], EDITORBASE+)

Attachments

(1 file, 4 obsolete files)

Update to Composer fix
2.32 KB, patch
Brade
: review+
kinmoz
: superreview+
asa
: approval+
Details | Diff | Splinter Review
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.
I can't reproduce this on Win 98....it worksforme...
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.
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
Confirm crash with todays build on Win98 SE
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Unable to Browse a page with composer second time. → Crash when browse a page with composer second time.
Keywords: crash
I just attatched a stack trace that shows we are crashing in nsDSURIContentListener::OnStartURIOpen(). I believe adamlock@netscape.com owns this.
Assignee: beppe → adamlock
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
Attached patch Patch to fix this crasher (obsolete) — — Splinter Review
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
cc charley
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?
I think we have to fix this in C++, not hack around it with JS changes. It seems to be an XPToolkit bug.
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.
yeah, window.openDialog should assert, and then either throw an exception or do absolutely nothing...
*** Bug 71244 has been marked as a duplicate of this bug. ***
adding shaver to the cc list. anthonyd
nom. moz0.9. note: I can repro this on win2k w/ build from yesterday.
Keywords: mozilla0.9
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.
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.
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
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.
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.
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
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
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.)
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.
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.
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.
*** Bug 41582 has been marked as a duplicate of this bug. ***
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.
*** Bug 73844 has been marked as a duplicate of this bug. ***
Severity: normal → critical
OS: Windows 95 → All
Hardware: PC → All
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?
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.
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?
Erh, not docshell, but whatever it is that represents the xul file being loaded/replaced.
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; }
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);
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.
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.
Target Milestone: --- → mozilla0.9.1
*** Bug 78184 has been marked as a duplicate of this bug. ***
CC'ing Rick. Rick, does any of your window targettting work touch on this problem?
Target Milestone: mozilla0.9.1 → mozilla0.9.2
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.
Retargetting. CC'ing jst & hyatt for ideas on why global variables are not in the script context the second time around.
Target Milestone: mozilla0.9.2 → mozilla0.9.3
*** Bug 87993 has been marked as a duplicate of this bug. ***
*** Bug 90925 has been marked as a duplicate of this bug. ***
** 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?
Keywords: correctness
Summary: Crash when browse a page with composer second time. → A blank window opens when clicking on the Browse button the 2nd time
Target Milestone: mozilla0.9.3 → mozilla0.9.4
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.
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.
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.
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 **********
Target Milestone: mozilla0.9.4 → mozilla0.9.5
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.
We're still crashing? (Doesn't seem to in Windows) Sure wish this was fixed for 0.9.4!
Retargetting
Adding status designations to keep our attention on this problem.
Whiteboard: [QAHP], EDITORBASE
0.9.5 is out the door. bumping TM up by one.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Retargetting. If someone has a good idea to fix this bug please take it.
Target Milestone: mozilla0.9.6 → mozilla1.0
Marking nsbeta1+ per syd. Need to fix for composer usability.
Keywords: nsbeta1+
*** Bug 95645 has been marked as a duplicate of this bug. ***
Works for me on mac os x build 2001121805, using both original steps and revised steps as noted in Comment 2.
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.
Attachment #20722 - Attachment is obsolete: true
Attachment #25448 - Attachment is obsolete: true
reproduced in Linux, marking +, adam, what's the chance of a fix by mozilla 1.0?
Whiteboard: [QAHP], EDITORBASE → [QAHP], EDITORBASE+
Attached patch Fix in Composer code (obsolete) — — Splinter Review
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.
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.
Assignee: adamlock → cmanske
Keywords: crash → patch, review
Whiteboard: [QAHP], EDITORBASE+ → [QAHP], EDITORBASE+, FIX IN HAND, need r=,sr=
Attached patch Update on Composer fix. (obsolete) — — Splinter Review
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.
Attachment #72513 - Attachment is obsolete: true
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?
Attachment #72514 - Flags: needs-work+
Put all interface-related calls inside the try
Attachment #72514 - Attachment is obsolete: true
Comment on attachment 72613 [details] [diff] [review] Update to Composer fix r=brade
Attachment #72613 - Flags: review+
Comment on attachment 72613 [details] [diff] [review] Update to Composer fix sr=kin@netscape.com 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.
Attachment #72613 - Flags: superreview+
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.
Status: NEW → ASSIGNED
Whiteboard: [QAHP], EDITORBASE+, FIX IN HAND, need r=,sr= → [QAHP], EDITORBASE+, FIX IN HAND, reviewed
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
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 on attachment 72613 [details] [diff] [review] Update to Composer fix a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #72613 - Flags: approval+
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!
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Whiteboard: [QAHP], EDITORBASE+, FIX IN HAND, reviewed → [QAHP], EDITORBASE+
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.
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: