Closed
Bug 308438
Opened 19 years ago
Closed 19 years ago
[FIX]Unavailability to intercept nsIDocShell::Destroy()
Categories
(Core :: DOM: Navigation, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.8beta5
People
(Reporter: spase, Assigned: bzbarsky)
Details
(Keywords: fixed1.8)
Attachments
(1 file, 1 obsolete file)
8.25 KB,
patch
|
Biesinger
:
review+
asa
:
approval1.8b5+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•19 years ago
|
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
Assignee | ||
Comment 1•19 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?
Assignee | ||
Comment 2•19 years ago
|
||
Or perhaps more precisely when an nsIWebNavigation is destroyed?
Comment 3•19 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.
Comment 4•19 years ago
|
||
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).
Assignee | ||
Comment 6•19 years ago
|
||
Attachment #196059 -
Flags: superreview?(darin)
Attachment #196059 -
Flags: review?(cbiesinger)
Updated•19 years ago
|
Attachment #196059 -
Flags: superreview?(darin) → superreview+
Comment 7•19 years ago
|
||
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?
Assignee | ||
Comment 8•19 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?
Assignee | ||
Comment 9•19 years ago
|
||
Attachment #196059 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #196059 -
Flags: review?(cbiesinger)
Assignee | ||
Updated•19 years ago
|
Attachment #196086 -
Flags: review?(cbiesinger)
Updated•19 years ago
|
Attachment #196086 -
Flags: review?(cbiesinger) → review+
Assignee | ||
Updated•19 years ago
|
Assignee: adamlock → bzbarsky
Priority: -- → P1
Summary: Unavailability to intercept nsIDocShell::Destroy() → [FIX]Unavailability to intercept nsIDocShell::Destroy()
Target Milestone: --- → mozilla1.8beta5
Assignee | ||
Comment 10•19 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?
Updated•19 years ago
|
Attachment #196086 -
Flags: approval1.8b5? → approval1.8b5+
Updated•19 years ago
|
Flags: blocking1.8b5? → blocking1.8b5+
Assignee | ||
Comment 11•19 years ago
|
||
Fixed, trunk and branch. Should we document this somewhere?
Comment 12•19 years ago
|
||
absolutely. how about some page linked from http://developer.mozilla.org/en/docs/Extensions? Maybe [[Intercepting browser events]], which would describe this + nsIWebProgressListener
Assignee | ||
Comment 13•19 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.
Description
•