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

RESOLVED FIXED in mozilla1.8beta5



13 years ago
13 years ago


(Reporter: spase, Assigned: bzbarsky)



1.8 Branch
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



13 years ago
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


13 years ago
Severity: normal → enhancement
Version: unspecified → 1.5 Branch


13 years ago
Assignee: nobody → adamlock
Severity: enhancement → normal
Component: General → Embedding: Docshell
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → adamlock
Version: 1.5 Branch → 1.8 Branch

Comment 1

13 years ago
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...
Flags: blocking1.8b5?

Comment 2

13 years ago
Or perhaps more precisely when an nsIWebNavigation is destroyed?

Comment 3

13 years ago
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.
I think we should provide a topic for webnavigation-created too. our current
mechanisms (nsIWPL, window created) are not optimal.

Comment 5

13 years ago
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

13 years ago
Created attachment 196059 [details] [diff] [review]
Something like this?
Attachment #196059 - Flags: superreview?(darin)
Attachment #196059 - Flags: review?(cbiesinger)


13 years ago
Attachment #196059 - Flags: superreview?(darin) → superreview+
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

13 years ago
> 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

13 years ago
Created attachment 196086 [details] [diff] [review]
Updated to comments
Attachment #196059 - Attachment is obsolete: true


13 years ago
Attachment #196059 - Flags: review?(cbiesinger)


13 years ago
Attachment #196086 - Flags: review?(cbiesinger)
Attachment #196086 - Flags: review?(cbiesinger) → review+


13 years ago
Assignee: adamlock → bzbarsky
Priority: -- → P1
Summary: Unavailability to intercept nsIDocShell::Destroy() → [FIX]Unavailability to intercept nsIDocShell::Destroy()
Target Milestone: --- → mozilla1.8beta5

Comment 10

13 years ago
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.
Attachment #196086 - Flags: approval1.8b5?


13 years ago
Attachment #196086 - Flags: approval1.8b5? → approval1.8b5+


13 years ago
Flags: blocking1.8b5? → blocking1.8b5+

Comment 11

13 years ago
Fixed, trunk and branch.

Should we document this somewhere?
Last Resolved: 13 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
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

13 years ago
Mm... I really won't be able to write anything that involved for a while...
(like "months" :( ).
You need to log in before you can comment on or make changes to this bug.