Closed Bug 308438 Opened 19 years ago Closed 19 years ago

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

Categories

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

1.8 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta5

People

(Reporter: spase, Assigned: bzbarsky)

Details

(Keywords: fixed1.8)

Attachments

(1 file, 1 obsolete file)

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
Severity: normal → enhancement
Version: unspecified → 1.5 Branch
Assignee: nobody → adamlock
Severity: enhancement → normal
Status: UNCONFIRMED → NEW
Component: General → Embedding: Docshell
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → adamlock
Version: 1.5 Branch → 1.8 Branch
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?
Or perhaps more precisely when an nsIWebNavigation is destroyed?
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.
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).
Attached patch Something like this? (obsolete) — Splinter Review
Attachment #196059 - Flags: superreview?(darin)
Attachment #196059 - Flags: review?(cbiesinger)
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
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?
> 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?
Attachment #196059 - Attachment is obsolete: true
Attachment #196059 - Flags: review?(cbiesinger)
Attachment #196086 - Flags: review?(cbiesinger)
Attachment #196086 - Flags: review?(cbiesinger) → review+
Assignee: adamlock → bzbarsky
Priority: -- → P1
Summary: Unavailability to intercept nsIDocShell::Destroy() → [FIX]Unavailability to intercept nsIDocShell::Destroy()
Target Milestone: --- → mozilla1.8beta5
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?
Attachment #196086 - Flags: approval1.8b5? → approval1.8b5+
Flags: blocking1.8b5? → blocking1.8b5+
Fixed, trunk and branch.

Should we document this somewhere?
Status: NEW → RESOLVED
Closed: 19 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
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.

Attachment

General

Created:
Updated:
Size: