Closed
Bug 88229
Opened 23 years ago
Closed 23 years ago
document.write not working for mfcembed (or any embeded Gecko)
Categories
(Core Graveyard :: Embedding: APIs, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla0.9.7
People
(Reporter: dunn5557, Assigned: danm.moz)
References
Details
(Whiteboard: [from bugscape] needed for embedding)
Attachments
(8 files, 3 obsolete files)
299 bytes,
text/html
|
Details | |
368 bytes,
text/html
|
Details | |
3.59 KB,
patch
|
Details | Diff | Splinter Review | |
2.58 KB,
patch
|
Details | Diff | Splinter Review | |
7.12 KB,
patch
|
Details | Diff | Splinter Review | |
4.86 KB,
patch
|
Details | Diff | Splinter Review | |
71.26 KB,
patch
|
rpotts
:
review+
|
Details | Diff | Splinter Review |
26.26 KB,
patch
|
Details | Diff | Splinter Review |
Notes moved from Bugscape (6874): ++++++++++++++++++++ With 0.9.2 branch. The following html page works properly on any browsers including standalone mozilla, but not on embedded Gecko. It was our last minute beta2 stopper. <SCRIPT LANGUAGE="JavaScript"> var popup = window.open ("about:blank","popup",'width=350,height=250,resizable=0'); var d = popup.document; d.write('<HTML><HEAD>'); d.write('<TITLE>This is your title</TITLE></HEAD>Helloo</html>'); d.close(); //--> </SCRIPT> <html> Hello1 </html> ------- Additional Comments From Eddie Xu 2001-06-26 12:15 ------- Created an attachment (id=2011) a test case ------- Additional Comments From Lisa Chiang 2001-06-26 12:49 ------- cc: marek qa to mdunn. Mike - pls see if there is any bugzilla bugs filed on this already. ------- Additional Comments From Eddie Xu 2001-06-26 13:18 ------- More research info; it is not the document reference not working, the problem is the returned popup window reference. Another weird thing is that if you do a reload (without closing the popup window), it will work. So my doubt is that at the moment window.open() returns, it is still in the middle of creating the docshell and hense the window reference is not valid. ------- Additional Comments From Marek Z. Jeziorek 2001-06-26 13:26 ------- Rick, I'm not sure if Adam is available soon (due to the time zones). Would you be able to investigate more before Adam is involved with this? --- thanks ------- Additional Comments From Adam Lock 2001-06-26 13:28 ------- I am here for an hour or so. My initial investigation makes me think this some kind of race condition. If I modify the test case to wait 3 seconds before writing it works. Modified version follows ------- Additional Comments From Adam Lock 2001-06-26 13:28 ------- Created an attachment (id=2012) Modified test case succeeds ------- Additional Comments From Adam Lock 2001-06-26 13:29 ------- Modified version is as below: <SCRIPT LANGUAGE="JavaScript"> var popup = window.open("about:blank","popup",'width=350,height=250,resizable=0'); setTimeout("splurge();", 3000); function splurge() { var d = popup.document; d.write('<HTML><HEAD>'); d.write('<TITLE>This is your title</TITLE></HEAD>Helloo</html>'); d.close(); } //--> </SCRIPT> <html> Hello1 </html> ------- Additional Comments From Eddie Xu 2001-06-26 13:52 ------- I am pretty sure we don't have this problem before (but not sure how long ago, at that time popup window did not render about:blank initially). I used to test stuff like popup = window.open("http://www.yahoo.com"); popup.resizeTo(200,300); and it used to work. It doesn't anymore. ------- Additional Comments From Adam Lock 2001-06-26 14:14 ------- Eddie, window methods should work immediately but the window.document property won't work until the document viewer has been created and associated with the window. And the document viewer won't be created until the uriloader tells the docshell's content listener that there is some content to render. This minor "gap" between the window opening and the document viewer being present. Perhaps we could make process atomic for about:blank, or wait in window.open for the first content to arrive before returning? Ideas? ------- Additional Comments From Eddie Xu 2001-06-26 14:58 ------- The two lines popup = window.open("http://www.yahoo.com"); popup.resizeTo(200,300); is the exact codes I tested and it does not work. Unfortunately I can not block inside nsIEmbeddingSiteWindow::CreateBrowserWindow and peek for document viewer because it will never be created before I return from CreateBrowserWindow. ------- Additional Comments From Rick Potts 2001-06-27 01:07 ------- hey Eddie, I've spent some time tracing through both your cases (popup.document and popup.resizeTo) and it turns out that they are related :-) In the popup.document case, it is just as Adam described - no document is available... Because nothing has loaded yet... In the popup.resizeTo case, things are a bit more funny :-) It is basically failing for the same reason - no document. However, in this situation, it is the Security Manager that is causing the failure... Basically, it is trying to verify that you have the right to call resizeTo(...). Unfortunately, since no document is available, it cannot locate a principal... This causes it to silently fail :-(. The funny part is that there is only a small window of time when you *are* allowed to call resizeTo... It will only succeed if it is called after about:blank has loaded, but before http://www.yahoo.com :-) This is due to security restrictions... The bottom line is that when ever you create a new browser window, you need to create a nested message pump and "block" until the initial document (probably about:blank) has been loaded into the webBrowser... You can take a look at how mozilla does it at: http://lxr.mozilla.org/seamonkey/source/xpfe/appshell/src/nsXULWindow.cpp#1291 As you also noticed, mfcEmbed does not do the right thing either when creating new windows (or with modality in general)... I'll see if we can fix up mfcEbbed to do the right thing as a simpler example :-) -- rick ------- Additional Comments From Eddie Xu 2001-06-27 07:45 ------- Thanks, Adam. 4517 does fix the painting problem, but not the DOM. In your test case (the one with setTimeout("splurge();", 3000)), if we change window.open ("about:blank") to window.open("") like most site does, your example won't work either. So this is what I am going to try on the embedding side: After a popup window is created, I am going to explicitly load about:blank and do a message pump, and wait for the first OnStateChange(NETWORK, DONE) to come in the new window, before I return from CreateBrowserWindow. Sounds like a right direction to go? <<Hi Eddie, The loading "about:blank" thing was a workaround while we sorted painting problems in new windows for bugscape 4517. This now appears to be fixed so I don't believe you need to do it anymore. Adam>> ------- Additional Comments From Eddie Xu 2001-06-27 09:08 ------- hmm...my initial try failed. Seems like if I do not return from CreateBrowserWindow, the doc loading on the new window will never get completed. Here is the codes: static void _WaitForDocShellReady() { CGbrowser* pNewBrowser = CGbrowser::_pNewBrowser; nsCOMPtr<nsIWebNavigation> pIWebNav; if ((pIWebNav = do_QueryInterface(pNewBrowser->m_pIWebBrowser))) { CWebBrowserContainer* pWebContainer = pNewBrowser- >m_pWebBrowserContainer; //Set it to true so that we can capture the document done event pWebContainer->m_bLoadingAboutBlank = TRUE; ::ResetEvent(pWebContainer->m_hDocShellReadyEvent); pIWebNav->LoadURI(L"about:blank", nsIWebNavigation::LOAD_FLAGS_NONE); _WaitForEvent(pWebContainer->m_hDocShellReadyEvent, 5000); } return; } NS_IMETHODIMP CWebBrowserContainer::OnStateChange(nsIWebProgress* aWebProgress, nsIRequest *aRequest, PRInt32 progressStateFlags, nsresult aStatus) { DebugStatusChange(aRequest, progressStateFlags); if (m_bLoadingAboutBlank) { if ((progressStateFlags & STATE_STOP) && (progressStateFlags & STATE_IS_NETWORK)) { ::SetEvent(m_hDocShellReadyEvent); m_bLoadingAboutBlank = FALSE; return NS_OK; } else return NS_OK; } ... } ------- Additional Comments From Adam Lock 2001-06-27 09:37 ------- Eddie, it may be possible to put some code into docshell that ensures that about: protocols are loaded synchronously. I have made an initial attempt to do this which does appear to work but it crashes almost immediatly after in the parser. I'll attach a patch of my change in the hope that someone might be able to shed light on the reason for the crash. To me it looks like the http channel is sending data after I have already received the network stop notification via the docshell's web progress listener. Stack trace is below. sParser::ResumeParse(int 0x00000001, int 0x00000000) line 2072 + 6 bytes nsParser::OnDataAvailable(nsParser * const 0x02b6b2f0, nsIRequest * 0x02bb3b80, nsISupports * 0x00000000, nsIInputStream * 0x02b728e8, unsigned int 0x00000000, unsigned int 0x00000127) line 2517 + 19 bytes nsDocumentOpenInfo::OnDataAvailable(nsDocumentOpenInfo * const 0x02bd6968, nsIRequest * 0x02bb3b80, nsISupports * 0x00000000, nsIInputStream * 0x02b728e8, unsigned int 0x00000000, unsigned int 0x00000127) line 235 + 46 bytes nsStreamListenerTee::OnDataAvailable(nsStreamListenerTee * const 0x02c1d070, nsIRequest * 0x02bb3b80, nsISupports * 0x00000000, nsIInputStream * 0x02bd76c8, unsigned int 0x00000000, unsigned int 0x00000127) line 56 + 51 bytes nsHttpChannel::OnDataAvailable(nsHttpChannel * const 0x02bb3b84, nsIRequest * 0x02ab91f8, nsISupports * 0x00000000, nsIInputStream * 0x02bd76c8, unsigned int 0x00000000, unsigned int 0x00000127) line 2144 + 57 bytes nsOnDataAvailableEvent::HandleEvent() line 175 + 70 bytes nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x02b729a4) line 64 PL_HandleEvent(PLEvent * 0x02b729a4) line 590 + 10 bytes PL_ProcessPendingEvents(PLEventQueue * 0x010353c8) line 520 + 9 bytes _md_EventReceiverProc(HWND__ * 0x001505da, unsigned int 0x0000c145, unsigned int 0x00000000, long 0x010353c8) line 1071 + 9 bytes USER32! 77e148dc() USER32! 77e14aa7() USER32! 77e266fd() CWinThread::Run() line 480 + 11 bytes CWinApp::Run() line 400 AfxWinMain(HINSTANCE__ * 0x00400000, HINSTANCE__ * 0x00000000, char * 0x00133d96, int 0x00000001) line 49 + 11 bytes WinMain(HINSTANCE__ * 0x00400000, HINSTANCE__ * 0x00000000, char * 0x00133d96, int 0x00000001) line 30 WinMainCRTStartup() line 330 + 54 bytes KERNEL32! 77e992a6() ------- Additional Comments From Adam Lock 2001-06-27 09:38 ------- Created an attachment (id=2021) Patch adds message look after channel load to wait for about: documents to finish loading - crashes later on in the parser! ------- Additional Comments From Rick Potts 2001-06-28 00:22 ------- hey Eddie, I ran into the same problem when I tried to add a sub-message-pump into mfcEmbed :-( It turns out that before entering the nested event loop a new nsIEventQueue must also be pushed !!! I'm attaching a patch to mozilla/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp which pushes the new event queue before calling nsIWebBrowserChrome::CreateBrowserWindow(...) This seems to fix the problem... ------- Additional Comments From Rick Potts 2001-06-28 00:24 ------- Created an attachment (id=2032) patch to push an nsIEventQueue before calling nsIWebBrowserChrome::CreateBrowserWindow(...) ------- Additional Comments From Eddie Xu 2001-06-28 08:08 ------- Cool! Initial test seems to work, will do some more testing...
Reporter | ||
Comment 1•23 years ago
|
||
Reporter | ||
Comment 2•23 years ago
|
||
comments from Eddie: "Patch is working and stable. Thanks Rick, could you push it into the trunk as a 0.9.2 patch? We will want to get it as soon as possible." adding keyword for mozilla0.9.2
Keywords: mozilla0.9.2
Comment 5•23 years ago
|
||
Comment 6•23 years ago
|
||
Comment 7•23 years ago
|
||
hey Chak, i've just attached a quick patch to mfcEmbed to fix this issue... Can you take a look at it - and maybe clean it up a bit :-) thanks, -- rick
Comment 8•23 years ago
|
||
Hi Rick : My MfcEmbed build dir is in kind of a mess right now due to some other bugs i'm working on. However, your changes to mfcembed look good to me. I'm guessing you've tried it and it works? :-) only other thing that would be helpful is a reference to this bug on why we're doing the PumpMessage(): + // + // Now block until an initial document (about:blank) has finished loading + // See http://bugzilla.mozilla.org/show_bug.cgi?id=88229 for more info. [Basically add the line above to the comments you already have] Thanks Chak
Updated•23 years ago
|
Target Milestone: --- → mozilla0.9.2
Comment 10•23 years ago
|
||
pushing out. 0.9.2 is done. (querying for this string will get you the list of the 0.9.2 bugs I moved to 0.9.3)
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Comment 11•23 years ago
|
||
Is there some way we can do this internally and spare embeddors from having to do this? This is nasty. To do this internally, we'd need two things: (1) the docshell tree owner would have to be an nsIWebProgressListener (at least temporarily) so it could know when the first load of about:blank has completed. (2) There would have to be a method (on the chrome, maybe) called PumpMessage(). Since this is app-specific, the embeddor would have to implement it and the internal code would call it while waiting for the load to happen. Having the embeddor just have to implement PumpMessage() seems better than having them do all this. Another advantage of having the internal code be an nsIWebProgressListener would be that we could also use it to handle the case of implicitly sized chrome dialogs for them. Of course, if this has to happen today and this is the only way out, the patch looks good. Let me try it out first.
Comment 12•23 years ago
|
||
Perhaps we need an NS_EmbedWaitForContent(nsIWebBrowser *browser) function?
Comment 13•23 years ago
|
||
Actually, we do have something XP and already available for the purpose of a blocking event loop - nsIWebBrowserChrome::ShowAsModal(). That method I think is mis-named. All my implementation does is enter a modal loop. It does not set the visibility of the window as the name implies because that's taken care of elsewhere. So, if we rename ShowAsModal -> EnterModalEventLoop (matches the existing ExitModalEventLoop), that could be used by the docshell tree owner to do this wait until load. I'm not sure how other people are using ShowAsModal though, but it's damn close to what we need.
Comment 14•23 years ago
|
||
Yes... Vidur and I were thinking the same thing... We should be able to leverage ShowAsModal (with a name change) to make this work... Then the DocShellTreeOwner could spin up a nested message pump until the DocShell was "ready"... Unfortunately, this still doesn't solve the "generic" problem that anyone else who calls nsIWebBrowserChrome::CreateBrowserWindow(...) will be forced to do this too :-( I don't know what the "right" answer is... we can simply document this to make it clear what the semantics of CreateBrowserWindow(...) are... or we can force the implementation of CreateBrowserWindow(...) to spin up its own nested message loop... Either way, i think that this patch is a "small" step in the right direction :-)
Comment 15•23 years ago
|
||
> Unfortunately, this still doesn't solve the "generic" problem that anyone else
> who calls nsIWebBrowserChrome::CreateBrowserWindow(...) will be forced to do
> this too :-(
You're right. And one important case which would be missed is that of windows
made through nsIWindowCreator. I'm pretty sure it would bypass anything we could
do in the docshelltreeowner.
So the patch, which does put some burden on embeddors, is probably the best we
can do. r=ccarlen
Comment 16•23 years ago
|
||
Patch looks good to me too so r=adamlock, but I know this is going to come back and bite us...
Comment 17•23 years ago
|
||
> And one important case which would be missed is that of windows made through
> nsIWindowCreator. I'm pretty sure it would bypass anything we could do in the
> docshelltreeowner.
Just had another thought on this. Why does the nsIWebBrowserChrome make new
windows in the first place? We now have nsIWindowCreator. When
nsDocShellTreeOwner::GetNewWindow(...) is called, instead of calling up to the
chrome to make the window, it could use nsIWindowCreator. That would cover all
cases uniformly. nsIWindowCreator::CreateChromeWindow(...) takes a parent param
so the new window can be just as parented as if the chrome had made it. Doing
this, everything is bottlenecked through one place (good). Only problem is, the
impl of nsIWindowCreator is done by the embeddor, so this spinning up of a msg
queue and loading about:blank would still be on them (bad). There's a way around
that: Put CreateChromeWindow(...) on nsIWindowWatcher. This is the method that
would be called by anything creating a window. Nobody would ever call
nsIWindowCreator directly - it could only be called by windowwatcher. Now, we
can put the msg queue, loading of about:blank, and event loop inside
nsIWindowWatcher::CreateChromeWindow(). The implementation of
nsIWindowCreator::CreateChrome window does not have to change a bit :-)
Comment 18•23 years ago
|
||
Comment 19•23 years ago
|
||
*** Bug 90067 has been marked as a duplicate of this bug. ***
Comment 20•23 years ago
|
||
looks like we have a patch? can this be pushed in 9.2 still ?
Updated•23 years ago
|
Assignee: rpotts → danm
Comment 21•23 years ago
|
||
hey dan, can you take this one over? Basically, we need to funnel all "window creation" through the WindowWatcher... Then we can push a PLEventQ before calling out to the "embeddor"... This way the embeddor can load "about:blank" or whatever and enter a sub-message pump until the URI has finished... Ultimately we should be able do the sub-message loop ourselves via the Modal/ExitModal APIs in nsIWebBrowserChrome... But i'm not sure if we can do *all* of that for the branch :-) -- rick
Comment 22•23 years ago
|
||
I forgot to mention that the patch that i've attached *does not* handle the case where the new window is created via nsIWindowCreator... It turns out that this is the case for top level windows that doe not have a parent :-( So, right now, i'm hanging in mfcEmbed when the unknown content handler comes up, because it is created via nsIWindowCreator...
Comment 23•23 years ago
|
||
lets take the temporary fix on the branch
Comment 24•23 years ago
|
||
I've just checked in the patch (http://bugzilla.mozilla.org/showattachment.cgi?attach_id=40529) to nsDocShellTreeOwner on the 0.9.2 branch (only). This is a stop-gap fix until we can rework window creation to funnel through a single point... -- rick
Comment 25•23 years ago
|
||
Since the last patch that I checked in is incomplete - it only deals with windows created via nsIWebBrowserChrome... not nsIWindowCreator... i'm going to hold off checking in the patches to mfcEmbed (et. al.) ... Otherwise, windows created via nsIWindowCreator would stall :-( -- rick
Comment 26•23 years ago
|
||
-> danm for the REAL fix :-)
Assignee: rpotts → danm
Whiteboard: pdt+, needed for embedding → needed for embedding
Updated•23 years ago
|
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Comment 27•23 years ago
|
||
branch fix is good enough to take the topembed keyword off this bug. hoping for this to get resolved in a better way for 0.9.4... danm, does that look likely..
Keywords: topembed
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Comment 28•23 years ago
|
||
Repeating chofman's question for Danm . . . Does this look likely for 0.9.4?
Assignee | ||
Comment 29•23 years ago
|
||
I'm just getting back around to this one. Wasn't very worried about it for 0.9.4 since there was a temporary patch. I'll see what I can do for the ongoing 0.9.4 effort, but I expect to not be able to do anything for official Mozilla 0.9.4.
Comment 30•23 years ago
|
||
candidate for nsbranch+
Keywords: topembed
Whiteboard: needed for embedding → [from bugscape] needed for embedding
Comment 31•23 years ago
|
||
It looks like the first patch (to push a nested nsIEventQueue when CreateBrowserWindow is called) needs to be checked into the 0.9.4 branch now :-)
Comment 32•23 years ago
|
||
I'd like to get this into the 0.9.4 branch. What would it take to fix (even using workaround) it, review and super-review this (once you get r= and sr=, please change the status for the appropriate patch). BTW, which patch is the right one for 0.9.4 branch?
Updated•23 years ago
|
Comment 33•23 years ago
|
||
we need to check http://bugzilla.mozilla.org/attachment.cgi?id=40529&action=view into the 0.9.4 branch... i was planning on just checking this one into 0.9.4 because it already got the r/sr/a treatment when it went into the 0.9.2 branch (and we're just pulling the work forward):-) is this ok? or do i need to start over again with the reviews? -- rick
Updated•23 years ago
|
Whiteboard: [from bugscape] needed for embedding → [from bugscape] needed for embedding, PDT+
Comment 34•23 years ago
|
||
yes, pull it forward to 0.9.4 -thanks
Comment 35•23 years ago
|
||
i've just pulled the first patch onto the 0.9.4 branch... --rick
Comment 36•23 years ago
|
||
Adding Ken and removing Glen from the cc: list.
Comment 37•23 years ago
|
||
*** Bug 100235 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 38•23 years ago
|
||
I'm removing the nsbranch+ status marker. rpotts' patch (already checked into the nsbranch) fixes this bug in 95% of all cases. The only known remaining problems are for non-Mozilla (embedding) apps which use the Mozilla security UI (PSM), and also because potentially the 100% fix for this bug may involve an API change. Because of that last point, I'm concerned about the impact of the 100% fix and don't really want to put it in the 0.9.4 branch. And because of the first point, I believe I can safely leave it out. This still remains a high priority bug (a bit lower now), but I believe that it wants to be left out of the 0.9.4 branch. Dissent?
Keywords: mozilla0.9.2,
nsbranch+
Comment 39•23 years ago
|
||
sounds ok to me. jud?, conrad?, rick?
Comment 40•23 years ago
|
||
I agree with Dan's latest comment.
Comment 41•23 years ago
|
||
removing topembed. potts' patch is sufficient.
Keywords: topembed
Whiteboard: [from bugscape] needed for embedding, PDT+ → [from bugscape] needed for embedding
Comment 42•23 years ago
|
||
*** Bug 101109 has been marked as a duplicate of this bug. ***
Comment 43•23 years ago
|
||
and back to topembed. landing pattern assuming resolution == trunk, then branch du jour evaluation (currently 0.9.4).
Keywords: topembed
Comment 44•23 years ago
|
||
Is this going to make 0.9.5? I would love to have it in.
Assignee | ||
Comment 45•23 years ago
|
||
oh jeez; that's today. nope.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Comment 46•23 years ago
|
||
Assignee | ||
Comment 47•23 years ago
|
||
Thanks Matthew ... but the complete patch that I'm working on in between putting out other fires is expected to just handle the problem. Unique patches to individual clients shouldn't be required. Of course I'll keep your patch around (obviously), in case that doesn't happen.
Comment 48•23 years ago
|
||
*** Bug 103733 has been marked as a duplicate of this bug. ***
Comment 49•23 years ago
|
||
what are the chances of this making 094?
Reporter | ||
Updated•23 years ago
|
QA Contact: mdunn → bmartin
Assignee | ||
Comment 50•23 years ago
|
||
see my comments above dated 2001-09-24 15:55
Comment 51•23 years ago
|
||
*** Bug 97638 has been marked as a duplicate of this bug. ***
Comment 52•23 years ago
|
||
*** Bug 96187 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 53•23 years ago
|
||
Patch to fix attached below. This also consolidates the two methods for instantiating new windows: it removes nsIWebBrowserChrome::createBrowserWindow in favour of nsPIWindowWatcher::createChromeWindow (embedding API change alert!)
Assignee | ||
Comment 54•23 years ago
|
||
Reporter | ||
Comment 55•23 years ago
|
||
For David: embedding API change alert
Comment 56•23 years ago
|
||
OK, some comments. In the gtk code you are doing #ifdef 0 on functions. If you're going to remove them, remove them. - // We're done loading. - mChromeLoaded = PR_TRUE; You remove this code that sets the flag but that flag is required for other code in the widget. I think that this is probably wrong and needs to be looked at harder. Do things like going to a site that needs HTTP auth work? I suspect that they won't. What is this code in EmbedPrivate::GetPIDOMWindow() that checks for 'useful' content? A load is a load is a load, right? In EmbedWindowCreator::RunEventLoop() do you not want to push the event queue stack? I don't know much about the mac embedding code. +nsWindowWatcher::InitializeDocshell(nsIWebBrowserChrome *aBrowserChrome) Oh, now I get the about:blank thing. This is so ugly. It's going to cause problems down the road. What happens when you actually want to load about:blank as the first page? What is the real solution here?
Comment 57•23 years ago
|
||
I wish this sort of thing didn't have to be in embeddor's code: +Boolean CBrowserWindow::RealContent() { + + Boolean real = true; + yada yada + if (PL_strcmp(locationSpec, "about:blank") == 0) + real = false; + } + } Is there any way (even if we have to make an nsIWebBrowserInternal iface) we can prevent the embeddor's nsIWeProgressListener from ever seeing this business?
Assignee | ||
Comment 58•23 years ago
|
||
Taking objections in the order they were given, more or less: No problem. I'll remove the methods instead of #if-0-ing them. mChromeLoaded -- I meant to move it, not delete it. Good catch. It's back in now. Ook. I don't know whether I want to push another event queue in RunEventLoop. Maybe; thinking of a general use method. Past experience suggests that it's best to push the new queue before the operation requiring the new queue has begun, and dangerous to push a second queue. Like the similar method Show(As)Modal, I'm pushing the new queue earlier on, before the operation requiring the event loop was initiated, and just running with it in the event loop. Hmm. I think it's healthiest for our current usage the way it is, but you do point out what looks like a systemic weakness. Not that that's any surprise with this event queue stuff. Heh. about:blank doesn't seem to have any fans. I agree it's a pretty skanky "API." A better way might be to add a property to nsIWebBrowser; to encapsulate the knowledge of whether a particular "finished loading" event is real, so clients can ask that question directly. I don't mind doing that if you think it's better. Maybe I'm not being very clever, but I don't think we can hide the docshell's load finishing from embeddors' nsIWebProgressListeners. I mean, no doubt it's possible but I think it would require wiring in a bunch of fragile special case spaghetti code underneath the listener interface. I haven't investigated that but I'm not eager to try it. Another possibility is completely changing the means by which embeddors determine that the webshell has finished loading. Make it use nsIObserverService and notify clients when it knows it's good and bloody done. Not sure what I think about that one. I don't see how an internal version of nsIWebBrowser would help. What are you thinking of, Conrad? Rick and I have been thinking of a solution for the "what about embeddors who truly want to load about:blank and actually have their window work" problem. We were thinking of allowing the caller to specify a URI, and clobbering them with about:blank only if they said nothing (presumably that's just the second parameter to OpenWindow). I'm not sure what effect that has on the problem of multiple fake docshell loads. You might still potentially get one, but only if you didn't specify anything in particular. In that case, do you care? If not, the last three paragraphs may be moot. Sigh. Rick and I were going to smack that gnat when we came to it, but maybe it's already time. I'll look into it.
Comment 59•23 years ago
|
||
> I don't see how an internal version of nsIWebBrowser would help. What are you
> thinking of, Conrad?
What I was thinking was:
(1) Get a list of the currently attached progress listeners, keep a ref to them,
remove them, add the one which listens for the load of about:blank, do the load,
remove the added one, restore the original list of listeners.
or
(2) Disable all attached progress listeners, add the one which listens for the
load of about:blank, do the load, remove the added one, and then enable them again.
Either of those would take some additional method on nsIWebBrowser but, since
it's frozen, a new internal iface.
Comment 60•23 years ago
|
||
I think that by forcing the load of page in a new window we are probably trying to solve this incorrectly. Why is it that we have to do that? Why can't we force the model to be sane before returning? That model build has to be triggered by something, right? Why can't we trigger it so the model is sane without loading a page?
Assignee | ||
Comment 61•23 years ago
|
||
That's a really good question. Thought about that. The problem this bug describes is that the document hasn't been completely built in time. What I need to do to fix it is the same thing that loading about:blank does; create a document viewer. I'm not familiar with the code that sets it up in response to a load of content, but I do know that it's all set up to work in response to content streaming in asynchronously. Hyatt sez it might be cool to teach window creation to sneak a viewer in synchronously, but it seems like more trouble than it's worth. By the way, I recant one point in my big message above. I believe that embedding clients who want to load about:blank are in no danger from my patch. The changes to the embedding clients affect only chrome URLs. So, the window isn't sized to content or made visible only if they're loading about:blank in chrome -- content is unaffected. Since it doesn't make sense to load about:blank in chrome on purpose, and it's never done as far as I know, I think we're OK there. Still remaining is the problem that embedding clients are seeing dirty underbellies they shouldn't have to see. I'm still looking into hiding the load of about:blank.
Assignee | ||
Comment 62•23 years ago
|
||
Alright. I started on a patch to hide the loading of about:blank from the webshell, then talked to rpotts and discovered that this will be taken care of by bug 46856 (related to bug 99639). When that one's fixed, WebProgressListeners will have a flag -- off by default -- for whether to be notified of progress for about:blank. When that's working there won't be any need for the special casing in embedding clients that you guys are objecting to. In the meantime they will need to special-case, so I think my current patch stands as is, though with comments added to the about:blank special cases that this is temporary. What say? Ready to go in, then?
Comment 63•23 years ago
|
||
This seems to be such a hack to me. Programming by side effect or something.
Comment 64•23 years ago
|
||
Is there anything to be said for putting a hardcoded check for about:blank into nsDocShell::LoadURI() that bypasses all of this channel loading stuff and simply creates a blank HTML document viewer before returning? I imagine this would be a significant performance gain for new windows as well as fixing this bug. BTW There's also an unimplemented CreateAboutBlankContentViewer method that could be adapted for this purpose.
Comment 65•23 years ago
|
||
That's a good idea, I guess. However, it means that if you explicitly load about:blank you won't get the notifications that a channel is loaded. Why not just have a call that creates the empty viewer without all of this about:blank crap? Also, what about cases where the new document that needs to be loaded isn't HTML? Does XML still use the same viewer? What about other types?
Assignee | ||
Comment 66•23 years ago
|
||
Both the last two comments are simply repeating questions already asked and answered. See my previous comment (except for the HTML vs XML question and yes, this works for both).
Comment 67•23 years ago
|
||
No, you didn't. You said that there was a way to hide the notifications for about:blank. I'm talking about not loading that at all, just force the creation of the content viewer. You don't have to worry about the listeners at all at that point, right? No special casing that way.
Assignee | ||
Comment 68•23 years ago
|
||
My mistake. Two comments up, now three. The paragraph that starts "That's a really good question..."
Comment 69•23 years ago
|
||
Bringing in dbaron, who knows lots about document viewers. I hope we can find a way without adding much code to make the on-demand "synchronous" case needed here work as well as our real-page-load "asynchronous" (non-blocking) case works. Can we not just cons up the needed data structures from within the document.write native code? /be
Assignee | ||
Comment 70•23 years ago
|
||
Alright. dbaron too professes ignorance. But I've managed to track down somebody who could give me some pointers. This is new territory for everyone, but I'm looking into the task of creating a document viewer truly synchronously without having to fake it.
Comment 71•23 years ago
|
||
Yea. Thank you for going into this unknown territory. I'll think we'll be glad you did it :-)
Comment 72•23 years ago
|
||
Is the bug summary here still valid?
Assignee | ||
Comment 73•23 years ago
|
||
The summary is arguably more constrained than the true boundaries of this bug's possible side effects, but it's accurate as far as it goes. --- Below is a preliminary patch. I'm asking for approval to get it checked in, disabled, in its current state, before it's quite working. This because it causes an embedding API change (it consolidates nsIWebBrowserChrome::CreateBrowserWindow with nsIWindowCreator::CreateChromeWindow). This is something we wanted to do anyway, and being an API change I'd like to get it into the trunk before it closes for 0.9.6 stability. I wouldn't want to surprise our embedding clients after then, and I do want to surprise them now. I haven't quite yet teased out the whole process of creating a valid document synchronously, so nDocShell::CreateAboutBlankContentViewer pretty much works, just not enough. I'd check in the patch with that disabled and everything would be normal (and still this bug not fixed). waterson has provisionally approved my change to nsIDocumentLoaderFactory, but he wanted adamlock or rpotts to look at that, too.
Assignee | ||
Comment 74•23 years ago
|
||
Comment 75•23 years ago
|
||
I'm not sure if I can see how it all fits together but, is it something like: nsWindowWatcher::InitializeDocshell() invokes CreateAboutBlankContentViewer() which just builds the thing?
I'm pretty sure this is basically the same root problem as bug 22681, where an IFRAME created through the DOM doesn't have a document attached to it ... because it's trying to load about:blank and the load event won't fire until the script is done.
Blocks: 22681
Comment 77•23 years ago
|
||
EmbedPrivate::ContentFinishedLoading still contains quite a few about:blank checks that I suspect are left over from the previous patch? I need to do some testing there to make sure that things aren't going to break. The code that you are playing with there is pretty fragile + // Asking Mozilla's docshell for its document will force it + // to generate one, and its content viewer. This is just what + // we need here. But it is a rather sneaky back door. + if (docshell) + nsCOMPtr<nsIDOMDocument> domdoc(do_GetInterface(docshell)); Don't program by side effect! :) What should I be looking at here as the long term stuff. The code that's in INITSYNCH or not? I'm guessing that we want the code inside of the ifdef since the code that forces the about:blank load is in the #else clause of that ifdef.
I was actually working on this during the weekend, but unsuccessfully. I hacked nsURILoader so that the document loader checks, as soon as it's created deep within LoadURI, whether the channel has a content-type available, and if so calls DispatchContent right away, which means the document gets created. Everything seemed to keep working fine and a newly created IFRAME element gets a document right away. Unfortunately something was still wrong; modifications to the document using write() and DOM methods didn't seem to do anything. I guess the document was still in a bad state. It would be great if LoadURI("about:blank") just consed up a blank document synchronously and bypassed all the elaborate loading machinery. I would have done that but I don't know this area of the code.
Assignee | ||
Comment 79•23 years ago
|
||
Conrad: yes basically, you've put your finger on the crux. Christopher: it is programming by side effect, as pointed out in a comment right there. However, I'm not even certain I need to do that. The patch builds in support for lazily creating an about:blank document synchronously if anyone ever asks for it. Then I'm explicitly asking for it (in a roundabout way). It's not yet working well enough for me to know whether I'll even have to do that. It seems likely that I won't. So I didn't want to put a lot of effort into making it explicit, with some new API call. Christopher: Oops! The #ifdef INITSYNCH stuff shouldn't be checked in. I have that switch installed right now so I can easily switch back and forth between the two methods of doing this and compare the code execution. The #ifndef INITSYNCH way includes a very Windows-specific hack to run a local event loop while loading about:blank. Definitely not part of the check-in picture. And yes, the about:blank check in EmbedPrivate::ContentFinishedLoading should be removed. I caught all the others but missed that one for some reason. Gone from my version now. Robert: I don't know this code very well, either. Turns out no one does. These two bugs do sound the same. I'm at a similar point with my patch as it stands: it does create a document which looks healthy unless you get too close. That's the part that still needs work and will be checked in disabled until I figure out what's missing. As noted in the first paragraph up there, I'm still not sure exactly how I want to trigger the creation of this document. Maybe on LoadURI("about:blank"), maybe explicitly any time a new webshell is created (and then several explicit loads of "about:blank" can be removed, probably at a performance benefit). Maybe lazily.
Comment 80•23 years ago
|
||
Can you post a new patch that has your changes in it so I can take it for a real test drive?
Assignee | ||
Comment 81•23 years ago
|
||
It's useable as-is, with INITSYNCH #defined (as it is). Just pull EmbedPrivate.cpp out of the patch; don't patch that file at all.
Comment 82•23 years ago
|
||
> I'd check in the patch with that disabled and everything would be normal (and
> still this bug not fixed)
Could this wait until the righteous path of CreateAboutBlankContentViewer() is
finalized? Having two different methodologies at work and #ifdef'ing them is
hard to understand. As important as this is to embedding, wouldn't this go into
0.9.4 branch (regardless of the 0.9.6 milestone ending) and then the trunk when
it re-opens?
Comment 83•23 years ago
|
||
looks good... r=rpotts@netscape.com (take a look at my 'nit-picking' comments :-) ) 1. in nsDocShell::CreateContentViewer(...) it looks like the PR_snprintf(...) can be removed... can't we just call: do_CreateInstance(NS_DOCUMENT_LOADER_FACTORY_CONTRACTID_PREFIX "view;1?type=text/html"); 2. you might as well delete the code inside of the #if 0 block in photon/src/WebBrowserContainer.cpp 3. in winEmbed.cpp you will want to remove the #if 0 block around szFirstURL so the home page is '.../projects/embedding' again... 4. remove the #if 0 code in EnableParent(...) in webshell/tests/viewer/nsWebBrowserChrome.cpp 5. is there a reason that nsContentTreeOwner::GetWebBrowser(...) in appshell no longer ASSERTs? 6. remove the #if 0 code in nsWindowCreator::CreateChromeWindow(... )in webshell/tests/viewer/nsWindowCreator.cpp
Comment 84•23 years ago
|
||
Comment on attachment 56587 [details] [diff] [review] preliminary: synchronously load docshell without event loops r=rpotts@netscape.com
Attachment #56587 -
Flags: review+
Assignee | ||
Comment 85•23 years ago
|
||
Conrad: The 0.94 discussion has been going on elsewhere. Current plan is to not move it to 0.94, but that could change. I imagine we'll know more once I finally figure out what's missing. I'd rather check it in somewhat disabled. The largest part of the patch is an API change that has been wanting to go in for a month now and would in my opinion cause unexpected hardship on embedding clients who may try to track the change through a stability freeze cycle, when everyone already has their work cut out for them. I could wait for 0.97 to open but I don't want to keep holding the API change. Rick: ah, thanks for actually reading the patch. Nobody likes my precious #if 0s. Alright, I'll kill them, except for #6, which I think wants to stay as a "wants fixing" kind of thing. I'll XXX it.
Assignee | ||
Comment 86•23 years ago
|
||
(oh, and about Rick's #5: that method no longer asserts because now I'm actually using it. It asserted before to complain if someone actually tried to use it.)
Comment 87•23 years ago
|
||
Dan, I didn't really review this patch (I could if you want me to), but I browsed through the first part of it and I noticed this: +nsContentDLF::CreateBlankDocument(nsIDocument **aDocument) +{ ... + // generate an html head element + nsCOMPtr<nsIHTMLContent> headElement; + nim->GetNodeInfo(nsHTMLAtoms::html, 0, kNameSpaceID_None, nsHTMLAtoms::html should be nsHTMLAtoms::head + *getter_AddRefs(htmlNodeInfo)); + NS_NewHTMLHeadElement(getter_AddRefs(headElement), htmlNodeInfo);
Comment 88•23 years ago
|
||
Dan, the patch looks reasonable, but can you raise bugs on any embedding apps you "break" by these API changes? I'm thinking mostly of the removal of nsIWebBrowserChrome::createBrowserWindow on the embedding apps in favour of the nsIWindowCreator method when some (e.g. the ActiveX control) had specific functionality in there.
Attachment #54146 -
Attachment is obsolete: true
Assignee | ||
Comment 89•23 years ago
|
||
The above patch was checked in Monday, so you're soaking in it as we speak. Adam: I think all the embedded apps in the tree have been correctly updated by the patch, but I'll make reminder bugs, at least. New news: I do believe I have it figured out. I'm attaching a patch that adds to the above patch and as far as I can tell successfully creates a perfectly good about:blank document on demand. It also only does this lazily, when someone requests the document but it isn't available. That part is a little iffy, and I think I'd recommend against putting it into the 0.96 branch at this point. I've been running with it and watching it in the debugger and I've seen no problems, but ... (Note if the desire to get this bug fixed in the 0.96 branch is overwhelming, I could make a slightly different version that does it explicitly. Safer. But until then, I'm tentatively moving this to 0.97. Speak up if you dislike.) A next step would be to remove some of the explicit loads of about:blank, like in nsHTMLFrameInnerFrame::DoLoadURL. But that's another day.
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Assignee | ||
Comment 90•23 years ago
|
||
Comment 91•23 years ago
|
||
Was it your intention to remove the single call site to InitializeDocshell? Other than that the patch looks good to me. I have yet to take it out for a test drive but the paint job looks great!
Assignee | ||
Comment 92•23 years ago
|
||
Yup. InitializeDocshell is the way I was forcing initialization as part of window creation. This patch attempts, successfully I think, to make that unnecessary by initializing the docshell lazily. nsPIWindowWatcher::CreateChromeWindow also was removed for the same reason. With lazy inititialization it just simplifies to nsIWindowWatcher::CreateChromeWindow.
Comment 93•23 years ago
|
||
OK, then I'm missing something. When _does_ the docshell get created during the window creation call? When some bad bad man tries to get the DOM object for the first time?
Assignee | ||
Comment 94•23 years ago
|
||
Not during window creation at all. With this patch, it will behave just as it did before, unless someone asks a DOM Window for its Document, or someone asks a DocShell for its ContentViewer. In either case if the requested object doesn't exist, it makes up on the spot a document/docshell/contentviewer set based on an artificially created about:blank document. Those two places seemed to cover all cases. I pushed the patch off to 0.9.7 because by doing so I'm opening up the range of side effects to a much larger audience. I'm clearly hijacking an actual asynchronous load of about:blank in some cases. (Well, I was already doing that.) And you'd think there would be the possibility of an infinite loop which however hasn't materialized because of the order of object initialization. However there are several places in the code that will be happy to find themselves standing downwind of the side effects. Bug 22681 for instance, and new iframe initialization especially in the tab browser. It's a bigger win this way.
Comment 95•23 years ago
|
||
- In nsContentDLF::CreateBlankDocument(), check for either failure or null out param after calling GetNodeInfoManager(), it can fail, for out of memory reasons if nothing else. - Also in nsContentDLF::CreateBlankDocument(), you shouldn't need to call SetDocument() on headElement and bodyElement, AppendChildTo() will do that for you since you already set the document on the root node to which you're appending. - In nsDocShell::CreateAboutBlankContentViewer(), no need to PR_snprintf() the contractid into a local buffer, just pass the two strings to do_CreateInstance() (i.e. do_CreateInstance(NS_DOC..._PREFIX "view;1?type=text/html")). - In WebBrowserContainer.cpp, you're #if 0'ing out code, why not remove the code in stead? - Add some more null checks to nsWebBrowserChrome::GetInterface(), I don't see us being guaranteed to have a non-null mBroweserWindow n' so on. Or maybe that's a safe assumption? Then for the second patch... - This one I'm not sure about, do we need to make up a load group in the case where we synchronously create the blank document? What if someone does document.write("<html><body onload=...><img src=...>"); document.close();? We must make sure there's a loadgroup in the document in that case so that the onload doesn't fire before the image has loaded. Who would own that loadgroup? - In nsDocument.cpp: + if (!mDocumentURL) + NS_WARNING("no URL!"); use NS_WARN_IF_FALSE() to make the if-check not be compiled into optimized builds. Other than that, the changes look good to me, and I'm willing to land this and fix the loadgroup issue later if it's really a problem. r/sr=jst
Comment 96•23 years ago
|
||
hey dan, i agree with jst, i think that the new document being created is going to require a load group... it looks like nsIDocumentLoaderFactory::CreateInstanceForDocument(...) should probably take an nsILoadGroup as an argument... i know it doesn't take a load group right now, but it looks like that is a bug to me :-)
Comment 97•23 years ago
|
||
So, what's going on with this patch? I haven't seen any movement here in a bit.
Comment 98•23 years ago
|
||
hey dan, i think this is looking really good... i have one suggestion tho... i think that in nsDocShell::EnsureContentViewer(...) we should make sure that mIsBeingDestroyed is *not* set before creating the blank content viewer... this will make sure that we don't recreate a content viewer during some wierd situation when the window is being closed... other than that... r/sr=rpotts@netscape.com -- rick
Assignee | ||
Comment 99•23 years ago
|
||
Har. Well thanks guys for going over the patch. However, here's a new one. It's just the one you've already reviewed, but I believe I've incorporated all your suggestions and corrections, and I've thrown a loadgroup into the creation of the new about:blank document. Not exactly in the way suggested. I'm currently trying to figure out whether it helped -- it doesn't seem to have hurt, anyway. Feel like doing it again? As I sort of said, besides incorporating suggestions, I've really just added an nsILoadGroup parameter to (my new method) nsIDocumentLoaderFactory::CreateBlankDocument and handed it the loadgroup from the nsDocShell.
Attachment #56989 -
Attachment is obsolete: true
Assignee | ||
Comment 100•23 years ago
|
||
Har. Well thanks guys for going over the patch. However, here's a new one. It's just the one you've already reviewed, but I believe I've incorporated all your suggestions and corrections, and I've thrown a loadgroup into the creation of the new about:blank document. Not exactly in the way suggested. I'm currently trying to figure out whether it helped -- it doesn't seem to have hurt, anyway. Feel like doing it again? As I sort of said, besides incorporating suggestions, I've really just added an nsILoadGroup parameter to (my new method) nsIDocumentLoaderFactory::CreateBlankDocument and handed it the loadgroup from the nsDocShell.
Attachment #58008 -
Attachment is obsolete: true
Comment 101•23 years ago
|
||
r/sr=jst for the latest patch.
Assignee | ||
Comment 102•23 years ago
|
||
Fix has been in since the evening of the 15th. Doesn't seem to be causing any problems. I say I'm done with this very bad boy. Thanks to jst and rpotts for their help, and now that I'm done, blizzard for making me do it the right way.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 103•23 years ago
|
||
Woohoo!
Comment 104•23 years ago
|
||
I tried the latest galeon/mozilla (1.0/0.9.6) combination and it isn't working for me on an ugly javasscript bookmark. I'll see if I can't prune it down to something more palatable if anyone's interested. See also http://bugzilla.gnome.org/show_bug.cgi?id=59611 , but I doubt that's going to shed a bunch of insight.
Assignee | ||
Comment 105•23 years ago
|
||
I'm interested, sure. I'll want to be able to reproduce it in something small, like TestGtkEmbed. (Note that the testcase in gnome bug 59611 now works in TestGtkEmbed.)
Comment 106•23 years ago
|
||
I can't vouch for gtktestembed since I don't have it (sorry, I'm rpm stupid... explain?) but the following code *as a bookmark* works in mozilla, but not galeon: javascript: function traverse_frames(w){w1=window.open("", "test");w1.document.open();w1.document.writeln("<H1>TEST</H1>");w1.document.close();}traverse_frames(window) line breaks aren't mine :-) the javascript console on galeon is reporting "Error: w1.document has no properties". I don't know enough to know if this is a gtkembed thing or a galeon thing, please be gentle. :-)
Assignee | ||
Comment 107•23 years ago
|
||
Oh. This bug was fixed in 0.9.7; it's not on the 0.9.6 branch.
Updated•5 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•