308438 - [FIX]Unavailability to intercept nsIDocShell::Destroy()

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Sam]

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 [:bzbarsky]]

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 [:bzbarsky]]

Or perhaps more precisely when an nsIWebNavigation is destroyed?

Comment 3

[Benjamin Smedberg]

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

I think we should provide a topic for webnavigation-created too. our current
mechanisms (nsIWPL, window created) are not optimal.

Comment 5

[Sam]

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
mContentListener->SetParentContentListener(nsnull);

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 [:bzbarsky]]

Attachment: Something like this?

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

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

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 [:bzbarsky]]

> 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 [:bzbarsky]]

Attachment: Updated to comments

Comment 10

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Fixed, trunk and branch.

Should we document this somewhere?

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

absolutely. how about some page linked from
http://developer.mozilla.org/en/docs/Extensions? Maybe [[Intercepting browser
events]], which would describe this + nsIWebProgressListener

Comment 13

[Boris Zbarsky [:bzbarsky]]

Mm... I really won't be able to write anything that involved for a while...
(like "months" :( ).