Last Comment Bug 308438 - [FIX]Unavailability to intercept nsIDocShell::Destroy()
: [FIX]Unavailability to intercept nsIDocShell::Destroy()
: fixed1.8
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: 1.8 Branch
: All All
: P1 normal (vote)
: mozilla1.8beta5
Assigned To: Boris Zbarsky [:bz]
: Adam Lock
Depends on:
  Show dependency treegraph
Reported: 2005-09-14 01:33 PDT by Sam
Modified: 2005-09-16 14:07 PDT (History)
10 users (show)
asa: blocking1.8b5+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Something like this? (5.95 KB, patch)
2005-09-14 11:21 PDT, Boris Zbarsky [:bz]
darin.moz: superreview+
Details | Diff | Splinter Review
Updated to comments (8.25 KB, patch)
2005-09-14 16:13 PDT, Boris Zbarsky [:bz]
cbiesinger: review+
asa: approval1.8b5+
Details | Diff | Splinter Review

Description Sam 2005-09-14 01:33:17 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Gecko/1.8b4

I made an application (in fact, this is AI Roboform Adapter) that connects to
current browser instance and works with its content. And, of course, I needed a
way to disconnect from browser (==nsIDocShell object) at the right moment.
In Gecko 1.7, there were objects nsIWebShell and nsIWebShellContainer. So I
registered my own WebShell container in corresponding WebShell.
On call to nsIDocShell::destroy, the framework released the pointer to
WebShellContainer. As this was my container, I got that and initiated disconnect
sequence - dropped all references to nsIDocShell in my code. This used to work OK.

In Gecko 1.8, there are no more nsIWebShell and, of course,
nsIWebShellContainer. And I hardly can disconnect from browser properly.

Reproducible: Always

Steps to Reproduce:

Expected Results:  
Have some way to intercept nsIDocShell::Destroy (event, release of stored
object, etc).

First found in Firefox/1.5b1
Comment 1 Boris Zbarsky [:bz] 2005-09-14 08:22:56 PDT
So I cannot in fact find a way to do this with our current code.  biesi, bryner,
darin, is there something I'm missing?

I'm thinking we should just send an observer service notification when a
docshell is destroyed...
Comment 2 Boris Zbarsky [:bz] 2005-09-14 08:23:35 PDT
Or perhaps more precisely when an nsIWebNavigation is destroyed?
Comment 3 Benjamin Smedberg [:bsmedberg] 2005-09-14 08:33:25 PDT
An observer topic sounds reasonable to me. This isn't going to be happening so
frequently that global notifications are going to be a perf problem.
Comment 4 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-14 09:00:04 PDT
I think we should provide a topic for webnavigation-created too. our current
mechanisms (nsIWPL, window created) are not optimal.
Comment 5 Sam 2005-09-14 10:59:53 PDT
In fact, I found a way to intercept nsDocShell::Destroy(), but I'm unsire this
is OK and this will work in future releases.

nsDocShell has a member
nsRefPtr<nsDSURIContentListener> mContentListener;

In nsDocShell::Destroy(), this is a call

And parent content listener is stored as nsWeakPtr, that is nullified by the
call above, forcing Release() to stored pointer. So I need to install my own
nsIURIContentListener (I anyway do that), but work with its refcounting
(specifically in Roboform adapter, this is possible). And if refcnt comes to
specific value, do disconnection routines.

Bad requirement: I should have incorrect implementation of GetWeakReference in
my code - it should in fact return object itself, not a reference. Thus I'll emulate

nsCOMPtr<nsIURIContentListener> parentListener

But this is just a hack, not a general solution, as it violates Gecko ideology.

BTW, if there would be a way to intercept window/frame creation, this would be
terrific. So far I use combinations of JS events and frame navigation events to
force rescanning of nsIDocShell tree (earlier, I fired this process by timer).
Comment 6 Boris Zbarsky [:bz] 2005-09-14 11:21:48 PDT
Created attachment 196059 [details] [diff] [review]
Something like this?
Comment 7 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-14 15:43:51 PDT
Comment on attachment 196059 [details] [diff] [review]
Something like this?

+		  "Unexpected item type in docshell");

trailing whitespace

is it good perf-wise to get a service upon each docshell creation?
I suppose we already do it for prefs, ok.

should Destroy have the same assertion as create? a docshell's type can

Is Destroy called more than once?

 NS_IMETHODIMP nsWebShell::Create()
+  if (mPrefs) {
+    // We've already been created

?? is this seriously called multiple times?

Is the type of the docshell set by the time Create is called?
Comment 8 Boris Zbarsky [:bz] 2005-09-14 16:06:20 PDT
> trailing whitespace

Will fix.

> is it good perf-wise to get a service upon each docshell creation?

It's really not that slow, and we don't create docshells that much.

> should Destroy have the same assertion as create?

Yes.  Will add.

> Is Destroy called more than once?

It can be.  It's an interface method.  And it's also called by ~nsDocShell.

> ?? is this seriously called multiple times?

It's also an interface method.  And even discounting that, it is: see
nsFrameLoader::EnsureDocShell and then nsSubDocumentFrame::ShowDocShell.  It
wasn't a huge deal till now because it was pretty idempotent.  That said, I
suspect the Create() call in ShowDocShell can go away...  For that matter, the
Create() impl in nsWebShell can go away, I think -- we never really use mThread.
 I'll file followup bugs on that.

> Is the type of the docshell set by the time Create is called?

Oh, nice catch.  "almost".  nsFrameLoader::EnsureDocShell and
nsWebBrowser::Create do it right, but nsWebShellWindow::Initialize doesn't.  All
the more reason to use nsWebBrowser everywhere...  I'll move the call in
nsWebShellWindow accordingly.  Want a new patch with that?
Comment 9 Boris Zbarsky [:bz] 2005-09-14 16:13:55 PDT
Created attachment 196086 [details] [diff] [review]
Updated to comments
Comment 10 Boris Zbarsky [:bz] 2005-09-16 09:04:12 PDT
Comment on attachment 196086 [details] [diff] [review]
Updated to comments

I think we should take this on the branch.  This is a very safe fix that should
allow embeddors and extension authors to avoid a lot of hackery that they have
to do now.
Comment 11 Boris Zbarsky [:bz] 2005-09-16 12:15:58 PDT
Fixed, trunk and branch.

Should we document this somewhere?
Comment 12 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-16 12:34:01 PDT
absolutely. how about some page linked from Maybe [[Intercepting browser
events]], which would describe this + nsIWebProgressListener
Comment 13 Boris Zbarsky [:bz] 2005-09-16 14:07:14 PDT
Mm... I really won't be able to write anything that involved for a while...
(like "months" :( ).

Note You need to log in before you can comment on or make changes to this bug.