Support loading of XUL overlays after the document has been displayed by using: var obs = { observe: function (subject, topic, data) { } }; document.loadOverlay("foo.xul", obs); obs is sent a "xul-overlay-merged" notification when the merge is complete. Note that this notification is sent ONLY to this observer, not out through the global observer service, since that would interfere with multiple instances of the same document window being open, for example. This implementation adds the loadOverlay method to nsIDOMXULDocument, implements it in nsXULDocument, factors XUL overlay loading code there to be shared between the two, handles post-document-construction merging and notification sending. There are various details, such as not sending the xul-overlay-merged message for dynamic overlays prior to initial layout, etc - which are documented in the patch.

(In reply to comment #0) > Support loading of XUL overlays after the document has been displayed Will this allow for installing extensions without restarting?

Not this patch in and of itself - it'd require hooks in EM, and probably extensions would have to make changes to make it work.

Comment on attachment 174196 [details] [diff] [review] patch >--- dom/public/idl/xul/nsIDOMXULDocument.idl 17 Apr 2004 21:50:12 -0000 1.6 >+++ dom/public/idl/xul/nsIDOMXULDocument.idl 13 Feb 2005 07:50:26 -0000 >@@ -60,9 +60,11 @@ interface nsIDOMXULDocument : nsISupport >+ >+ void loadOverlay(in DOMString url, in nsIObserver aObserver); There might be some value in making this be a nsIURI, but it's probably not a big deal, and this gives us symmetry with nsIDOMXMLDocument::load(). >--- content/base/src/nsDocument.cpp 2 Feb 2005 23:16:00 -0000 3.531 >+++ content/base/src/nsDocument.cpp 13 Feb 2005 07:50:28 -0000 >@@ -2866,22 +2866,21 @@ nsDocument::GetBoxObjectFor(nsIDOMElemen > { > NS_ENSURE_ARG(aElement); > > nsresult rv; > > *aResult = nsnull; > > if (!mBoxObjectTable) { >- mBoxObjectTable = new nsSupportsHashtable; >+ mBoxObjectTable = new nsInterfaceHashtable<nsVoidPtrHashKey, nsIBoxObject>; >+ mBoxObjectTable->Init(); You don't need to create the box object table here if it doesn't exist yet. SetBoxObject() will create it if a box object was successfully created. > } else { >- nsISupportsKey key(aElement); >- nsCOMPtr<nsISupports> supports = dont_AddRef(mBoxObjectTable->Get(&key)); >- >- nsCOMPtr<nsIBoxObject> boxObject(do_QueryInterface(supports)); >+ nsCOMPtr<nsIBoxObject> boxObject; >+ mBoxObjectTable->Get(NS_STATIC_CAST(void*, aElement), getter_AddRefs(boxObject)); You shouldn't need to cast the element like that... same in some other places. Also, in the code below, if (boxObject) { *aResult = boxObject; NS_ADDREF(*aResult); return NS_OK; } If it's not too much trouble, please change it to: if (boxObject) { boxObject.swap(*aResult); return NS_OK; } >@@ -2943,30 +2942,32 @@ nsDocument::GetBoxObjectFor(nsIDOMElemen > } > > NS_IMETHODIMP > nsDocument::SetBoxObjectFor(nsIDOMElement* aElement, nsIBoxObject* aBoxObject) > { > if (!mBoxObjectTable) { > if (!aBoxObject) > return NS_OK; >- mBoxObjectTable = new nsSupportsHashtable(12); >+ mBoxObjectTable = new nsInterfaceHashtable<nsVoidPtrHashKey, nsIBoxObject>; >+ mBoxObjectTable->Init(12); 12 is actually smaller than PL_DHASH_MIN_SIZE, which I suspect is bad. I'd say just use the default size. Also, needs to check for Init() failure. > } else { >- nsCOMPtr<nsISupports> supp; >- mBoxObjectTable->Remove(&key, getter_AddRefs(supp)); >- nsCOMPtr<nsPIBoxObject> boxObject(do_QueryInterface(supp)); >+ nsCOMPtr<nsIBoxObject> boxObject; >+ mBoxObjectTable->Get(NS_STATIC_CAST(void*, aElement), getter_AddRefs(boxObject)); > if (boxObject) { >- boxObject->SetDocument(nsnull); >+ nsCOMPtr<nsPIBoxObject> piboxObject(do_QueryInterface(boxObject)); >+ if (piboxObject) { >+ piboxObject->SetDocument(nsnull); >+ } > } Do we not need to do this SetDocument() stuff if we're replacing an existing box object? Or do we only ever set a box object once? (that's more likely, I guess) General comment before I start on nsXULDocument: are you sure we want to make the overlay load notification conditional on initial reflow? It seems like doing it after onload fires would make more sense, assuming that overlay references block onload. >--- content/xul/document/src/nsXULDocument.cpp 2 Feb 2005 23:16:02 -0000 1.644 >+++ content/xul/document/src/nsXULDocument.cpp 13 Feb 2005 07:50:32 -0000 >@@ -391,16 +392,21 @@ nsXULDocument::~nsXULDocument() > // decls never got resolved. > DestroyForwardReferences(); > > // Destroy our broadcaster map. > if (mBroadcasterMap) { > PL_DHashTableDestroy(mBroadcasterMap); > } > >+ if (mOverlayLoadObservers.IsInitialized()) >+ mOverlayLoadObservers.Clear(); >+ if (mPendingOverlayLoadNotifications.IsInitialized()) >+ mPendingOverlayLoadNotifications.Clear(); >+ These should clean themselves up on destruction automatically. >+NS_IMETHODIMP >+nsXULDocument::LoadOverlay(const nsAString& aURL, nsIObserver* aObserver) >+{ >+ nsresult rv; >+ >+ nsCOMPtr<nsIURI> uri; >+ rv = NS_NewURI(getter_AddRefs(uri), aURL, nsnull); >+ if (NS_FAILED(rv)) return rv; >+ >+ if (aObserver) { >+ nsCOMPtr<nsIObserver> obs; >+ if (!mOverlayLoadObservers.IsInitialized()) >+ mOverlayLoadObservers.Init(); >+ mOverlayLoadObservers.Get(uri, getter_AddRefs(obs)); I'd use GetWeak() here. >+PR_STATIC_CALLBACK(PLDHashOperator) >+FirePendingMergeNotification(nsIURI* aKey, nsCOMPtr<nsIObserver>& aObserver, void* aClosure) >+{ >+ if (aObserver) >+ aObserver->Observe(aKey, "xul-overlay-merged", EmptyString().get()); >+ >+ typedef nsInterfaceHashtable<nsURIHashKey,nsIObserver> table; >+ table* observers = NS_STATIC_CAST(table*, aClosure); >+ if (observers) Don't null check this, it can never be null since it's the address of a member. I do plan to continue this review, but I'm taking a break and don't want to lose the comments.

Comment on attachment 174196 [details] [diff] [review] patch >- mBoxObjectTable = new nsSupportsHashtable(12); >+ mBoxObjectTable = new nsInterfaceHashtable<nsVoidPtrHashKey, nsIBoxObject>; please don't forget to check for allocation failure (that the old code didn't is no excuse, you'll crash sooner than it used to) >+ mBoxObjectTable->Init(12);

Comment on attachment 174196 [details] [diff] [review] patch >+ if (!didInitialReflow) { Can you not use mInitialLayoutComplete here rather than asking the pres shell? >+ // If we have not yet displayed the document for the first time >+ // (i.e. we came in here as the result of a dynamic overlay load >+ // which was spawned by a binding-attached event caused by >+ // StartLayout() on the master prototype - we must remember that >+ // this overlay has been merged and tell the listeners after >+ // StartLayout() is completely finished rather than doing so >+ // immediately - otherwise we may be executing code that needs to >+ // access XBL Binding implementations on nodes for which frames >+ // have not yet been constructed because their bindings have not >+ // yet been attached. This can be a race condition because dynamic >+ // overlay loading can take varying amounts of time depending on >+ // whether or not the overlay prototype is in the XUL cache. The >+ // most likely effect of this bug is odd UI initialization due to >+ // methods and properties that do not work. >+ if (!mPendingOverlayLoadNotifications.IsInitialized()) >+ mPendingOverlayLoadNotifications.Init(); >+ mPendingOverlayLoadNotifications.Get(overlayURI, getter_AddRefs(obs)); If you just initialized mPendingOverlayLoadNotifications, then overlayURI won't be in the table. I'd put that part in an |else|. >+ if (!obs) { >+ mOverlayLoadObservers.Get(overlayURI, getter_AddRefs(obs)); >+ if (obs) You already null-check the observer when adding to the table, so you don't need to check it again here. Just have an NS_ASSERTION(). >@@ -3647,23 +3776,24 @@ nsXULDocument::OverlayForwardReference:: > nsAutoString tempID; > rv = aOverlayNode->GetAttr(kNameSpaceID_None, nsXULAtoms::id, tempID); > > // Element in the overlay has the 'removeelement' attribute set > // so remove it from the actual document. > if (attr == nsXULAtoms::removeelement && > value.EqualsLiteral("true")) { > >- rv = RemoveElement(aTargetNode->GetParent(), aTargetNode); >+ nsIContent* parent = aTargetNode->GetParent(); >+ rv = RemoveElement(parent, aTargetNode, aNotify); Is there a point to this change? sr=me with these and the changes mentioned in the previous comment.

Comment on attachment 174196 [details] [diff] [review] patch - In nsDocument::GetBoxObjectFor(): + mBoxObjectTable->Get(NS_STATIC_CAST(void*, aElement), getter_AddRefs(boxObject)); I guess you're not landing these changes with this patch, but I'll comment on them still... You're using the element pointer here as the key, it would be a good idea to QI that pointer to nsISupports before using it as the key since in some cases where we have multiple inheritance of nsIDOMElement you could get two different pointers for the same element into this method. Same thing all other places where you key off of an interface pointer. - In nsXULDocument::LoadOverlay(): + if (aObserver) { + nsCOMPtr<nsIObserver> obs; + if (!mOverlayLoadObservers.IsInitialized()) + mOverlayLoadObservers.Init(); + mOverlayLoadObservers.Get(uri, getter_AddRefs(obs)); + if (!obs) + mOverlayLoadObservers.Put(uri, aObserver); + } So this'll only add the observer if there isn't one in the table already. Then what happens if we get more than one call to load the same overlay? That's not something we care to support is it? If it is, then it seems like we should have the code to support firing off more than one observer notification for a given overlay (i.e. store a list of observers in the hash). The easier thing to do would of course be to explicitly say that we don't support that and throw an exception at the caller if the overlay is already loading. Other than that, and what bryner said, this looks good to me. I'll have one more look once there's a new diff...

(In reply to comment #6) > (From update of attachment 174196 [details] [diff] [review] [edit]) > >+ if (!didInitialReflow) { > > Can you not use mInitialLayoutComplete here rather than asking the pres shell? No. mInitialLayoutComplete is set after a whole bunch of stuff happens, whereas GetDidInitialReflow returns true much earlier in the process. The difference, it turns out is crucial. Why, I cannot say other than to say that the two things have different meanings and imply different sequencing.

Comment on attachment 175054 [details] [diff] [review] newer patch You didn't apply this change: ------------ >+PR_STATIC_CALLBACK(PLDHashOperator) >+FirePendingMergeNotification(nsIURI* aKey, nsCOMPtr<nsIObserver>& aObserver, void* aClosure) >+{ >+ if (aObserver) >+ aObserver->Observe(aKey, "xul-overlay-merged", EmptyString().get()); >+ >+ typedef nsInterfaceHashtable<nsURIHashKey,nsIObserver> table; >+ table* observers = NS_STATIC_CAST(table*, aClosure); >+ if (observers) Don't null check this, it can never be null since it's the address of a member. ------------ Everything else is fine.

Comment on attachment 175054 [details] [diff] [review] newer patch - In nsXULDocument::LoadOverlay(): + if (!obs) + mOverlayLoadObservers.Put(uri, aObserver); + else { + // We don't support loading the same overlay twice into the same + // document - that doesn't make sense anyway. + return NS_ERROR_FAILURE; + } You could save yourself an else-branch there if you'd flip that around like so: + if (obs) + // We don't support loading the same overlay twice into the same + // document - that doesn't make sense anyway. + return NS_ERROR_FAILURE; + } + + mOverlayLoadObservers.Put(uri, aObserver); but whatever, r=jst either way.

While we're adding methods to the IDL, could we PLEASE document them? A good start here would be documenting what notifications are issued to the observer and when, whether it's valid to pass a null observer, what happens if the overlay can't be loaded or if the url string cannot be converted to a uri (eg, do we throw synchronously, or notify the observer of failure asynchronously, or what?), whether the url string is allowed to be relative (to the document uri, presumably), what happens when one tries to load the same overlay while it's already loading, etc. Other issues with the patch off the top if my head: 1) Calling Init() on hashtables can (and will, since we finally have people testing these sort of things) fail. There need to be checks in this code for said failure. Same for all other places where hashtables are initialized. 2) The fact that the observer can't be null (what if I don't care about when the load finishes?) isn't documented, isn't asserted in LoadOverlay(), but _is_ randomly asserted in the guts of ResumeWalk(). 3) If the overlay fails to load, do we ever drop our ref to the observer? I don't see where we do it, and if we don't, there's a good chance that this leaks (since the observer presumably holds a ref to caller, which holds a ref to the XUL document). Note that failure to load can probably be caused by closing the window the overlay is loading into while it's loading (which will cancel the channel, etc). I'm going to take the liberty of reopening this bug pending resolution of these issues...

some people clearly don't care about notifications, and they merily crash :)

I am going to redo the way this notifies completion... basically using a XULOverlayMerged event so multiple event listeners within the context of a single document can observe. This will simplify the notifcation logic a bit.

Will this event fire synchronously from overlay merging? And if so, how well will we deal with an event handler closing the window or rearranging the DOM?

Note: this may have caused crash bug 284330

In fact, it did cause bug 284330 (tested via backing this patch out).

Boris, the intent was to fire the event(s) in the same conditions the observer notifications are now fired. i.e.: 1. if the overlay is merged after initial display, the event is fired when the merge is complete 2. if the overlay is merged prior to initial display, the event is fired after the initial display What are your concerns wrt closing the window?

(The primary reason for (2) is that we can't rely on bindings to be attached prior to frame construction, which was something I had heard you were interested in fixing... if that is indeed the case then barring any other problem I am overlooking the event could be fired as soon as the overlay is merged in all situations).

My concern with closing the window or rearranging the DOM is that the merging code may be making assumptions about the DOM not changing or certain data structures not getting destroyed while it's running. I'm not saying it definitely makes these assumptions; I'm just saying that this is something that needs to be carefully checked.

I'm having trouble getting the XUL Template builder to properly build menulists based on RDF datasources if the XUL template is inside a dynamic overlay. The template builder loads the datasource just fine but then it never trys to actually build the content.

This diff shows how a XULOverlayMerged event might work. I need to document this some more and make some test cases, but as a basic idea here you go.

I could be debugging the wrong thing, but it looks like the content is never built for the template because: http://lxr.mozilla.org/mozilla/source/content/xul/templates/src/nsXULContentBuilder.cpp#443 aTemplateNode->GetChildCount(); returns a value of 0 so nsXULContentBuilder::BuildContentFromTemplate never actually does any work.

ignore my comment about the child count being zero, I was debugging the wrong element. I do see us doing interesting stuff in BuildContentFromTemplate, it just isn't showing up in the UI. Still looking.

It looks like the template builder is properly creating the menu items and each menu item is getting a lazy state value of eChildrenMustBeRebuilt assigned to it. I'm wondering if with the dynamic overlay, no one comes along and forces us to rebuild since the template builder isn't creating these menu items until after the document has been loaded.

Interestingly enough, I can make the elements generated by the template builder show up if I force a notification after the template based elements get inserted into the the menupopop. I can do this by passing PR_TRUE into nsXULContentBuilder::BuildContentFromTemplate which alerts document observers when the items are added to the document. By passing in TRUE, later on in nsXULElement::InsertChildAt, we inform the document that an element has been added. I'm not sure what that means as far as fixing this for dynamic overlays, but it seems to prove the theory that the items get properly constructed and inserted into the document, but the document ignores the fact that they are there.

shaver and I came up with a fix for this. We took Ben's change to nsXULDocument which sets aNotify to TRUE if we have already done the initial reflow, and adds that same decision making step to the xul content builder. You can see the patch in Bug #285076. I asked brian and Ben to review since they were involved with writing/reviewing this patch. But if someone else (bz, jst) has an opinion, please feel free to jump in.

This patch caused regression crash: bug 284330 four weeks ago Per the zero tolerance policy, that means this patch should be backed out since noone has acted on that crash. anyone have any objections?

I wish we didn't have to back it out, because it'll break the pref window stuff on the trunk, but I don't see any progress on this, so I'm not sure we have a lot of choices. =/ Ben seems pretty busy (given that he seemed not to be able to spend any time on it last week, as per bug 284330 comment 14), so unless someone else steps up ASAP I think we need to revert. Bleh. I was just starting to use this stuff in lightning, too.

I said in the other bug I was looking at the regression this week.

OK. Now that the obvious crash regression is fixed, is there a chance of addressing the issues in comment 12? If they're to be addressed with a major rewrite of this core Gecko code, said rewrite probably needs to happen before 1.8b2 ships.

Boris: so what do you think of the approach in my "initial prototype" patch?

(it adds an "XULOverlayMerged" event which is fired when the overlay is merged - no need to maintain a single observer, and the system can support many listeners since it uses the event system)

Two thoughts off the top of my head: 1) Is there a way to get notified when the merge fails? 2) See comment 15 and comment 20. Keep in mind that event handlers can (and will, experience shows) execute arbitrary script.

OK, I'll take a look at this patch again with these things in mind.

I want to get something final here this week for the alpha release, so I'll be looking at this once I'm done landing my EM changes.

I realize that redesigning this is time-consuming, so perhaps we should fix things like the OOM handling and the leak-on-error and move the redesign to a different (Gecko 1.9?) bug?

Looks like this also caused bug 293460.

Ben, what needs to happen here to resolve this for 1.1?

I'll attach a patch updated to the current trunk once my tree is done rebuilding.

Sorry for the delay. I am waiting for IS to re-image my development machine after an unfortunate experience with Vista b1.

Ben - still thinking you can land today (8/10) in time for branch?

Mike, I don't have a reviewed patch yet. I am very close to a reviewed patch but need to have VS.NET installed on my machine. I will do this and prepare a patch tonight. This bug will fix the crash bug that's on my list. (which is farily easy to reproduce)

To answer bz's question: Updating the page dom in an event handler is not a problem - dom manipulation occurs constantly through the merge process since overlays can specify content removal/addition/modification using insertbefore/after/position and removeelement attributes. These operations use standard DOM APIs to work. This patch adds a new event XULOverlayMerged which clients can listen for to observe overlay merge completion.

You should modify the code in nsDOMEvent::GetEventName for the new event type. Please also add a reference to the new event type in the "e.g." comment in nsDOMEvent::GetEventName (since you're not modifying SetEventType, nor should you be, at this point). You may also want to add a method to the end of nsIDOMXULListener, and thus also to the code in nsEventListenerManager.cpp (sXULEvents, GetIdentifiersForType).

Comment on attachment 192454 [details] [diff] [review] updated to trunk patch >Index: content/xul/document/src/nsXULDocument.cpp >=================================================================== >@@ -3204,57 +3196,43 @@ nsXULDocument::ResumeWalk() > rv = group->RemoveRequest(mPlaceHolderRequest, nsnull, NS_OK); > if (NS_FAILED(rv)) return rv; > } > } > mInitialLayoutComplete = PR_TRUE; > > // Walk the set of pending load notifications and notify any observers. > // See below for detail. >- if (mPendingOverlayLoadNotifications.IsInitialized()) >- mPendingOverlayLoadNotifications.Enumerate(FirePendingMergeNotification, (void*)&mOverlayLoadObservers); >+ PRInt32 count = mPendingOverlayLoadNotifications.Count(); >+ for (i = 0; i < count; ++i) { >+ FireXULOverlayMergedEvent(mPendingOverlayLoadNotifications[i]); >+ mPendingOverlayLoadNotifications.RemoveObjectAt(i); >+ } This will fail if count > 1; RemoveObjectAt will delete the object and shift the array down, and the next index will be invalid. Can just use [0] and RemoveObjectAt(0) or iterate from count-1 .. 0.

(... or just don't call RemoveElementAt at all and just Clear the array after the loop)

The order of the enum in nsIDOMClassInfo is a frozen interface. You need to add to the end, not to the middle. (All the changes should be in the same position; for two of them, it actually matters that they match.) The code in nsXULDocument::FireXULOverlayMergedEvent seems like a slightly unusual way to fire events from C++, although nsDocument.cpp does it in a bunch of places. I'd think something more like what nsDocument::OnPageShow does might be better, although neither is quite as clean as it ought to be.

You should rev the UUID of nsIDOMXULDocument.

I was not able to add to nsIDOMXULListener since we've run out of bits in EventDispatchData (there are already 8 methods on nsIDOMXULListener and the type is stored in a PRUint8)... bryner said we don't really want to make every EventDispatchData struct bigger for no reason, and mentioned he was working on a patch not so long ago to make that aspect a little better. He suggested that if these events were only being listened to from JavaScript this inconsistency was not necessarily worth stopping this patch for for now at least. I will file a bug on the discrepancy if consensus is that that is reasonable.

rev uuid of nsIDOMXULDocument

bryner follows up with "Using PRUint8 actually won't make the structs any bigger since they will be padded to words... but changing to 16-bits is non-trivial anyway because of where this value may be used"

Comment on attachment 192470 [details] [diff] [review] patch This looks all except for one problem in nsXULDocument::ResumeWalk(). The only other things I had to comment on were minor style nits, fix as you check in or whatever: - In nsDOMEvent.cpp: - "DOMActivate", "DOMFocusIn", "DOMFocusOut", - "pageshow", "pagehide" + "DOMActivate", "DOMFocusIn", "DOMFocusOut", + "pageshow", "pagehide", "XULOverlayMerged" Whitespace added to the end of the first line there for no reason... case NS_PAGE_SHOW: return sEventNames[eDOMEvents_pageshow]; case NS_PAGE_HIDE: return sEventNames[eDOMEvents_pagehide]; + case NS_XULOVERLAYMERGED_EVENT: + return sEventNames[eDOMEvents_XULOverlayMerged]; Two-space indentation here... +nsDOMXULOverlayMergedEvent::InitXULOverlayMergedEvent(const nsAString & aTypeArg, [...] + switch (mEvent->eventStructType) + { + case NS_XULOVERLAYMERGED_EVENT: + { + nsXULOverlayMergedEvent* event = NS_STATIC_CAST(nsXULOverlayMergedEvent*, mEvent); + event->mOverlayURI = aOverlayURI; + NS_IF_ADDREF(event->mOverlayURI); + break; + } + default: + break; + } 3-space indentation? - In nsXULDocument::FireXULOverlayMergedEvent(nsIURI* aOverlayURI): + if (pc) { + nsXULOverlayMergedEvent event(PR_TRUE, NS_XULOVERLAYMERGED_EVENT); + nsEventStatus status = nsEventStatus_eIgnore; Fix indentation... - In nsXULDocument::ResumeWalk(): + for (i = 0; i < count; ++i) + FireXULOverlayMergedEvent(mPendingOverlayLoadNotifications[0]); + mPendingOverlayLoadNotifications.Clear(); This ends up firing the notifications for mPendingOverlayLoadNotifications[0] count times. You'll need a mPendingOverlayLoadNotifications.RemoveElementAt(0) in the loop too. And fix the indentation here too. - In nsIDOMClassInfo.h: @@ -78,17 +78,17 @@ enum nsDOMClassInfoID { [...] eDOMClassInfo_KeyboardEvent_id, eDOMClassInfo_PopupBlockedEvent_id, - + Whitespace added for no reason... sr=jst with that fixed.

(In reply to comment #54) > + for (i = 0; i < count; ++i) > + FireXULOverlayMergedEvent(mPendingOverlayLoadNotifications[0]); > + mPendingOverlayLoadNotifications.Clear(); I am a moron :-) -Ben

Comment on attachment 192470 [details] [diff] [review] patch Could you please make the event's |msg| different from the |eventStructType|? Having them be the same leads to confusion about what we're dealing with, and awhile back this was fixed for all of the existing event types. For example, make the actual message be NS_XULOVERLAY_MERGED, and define it down where the other message types are defined in nsGUIEvent.h.

fix whitespace, nsXULOverlayMerged initialization (per bryner), and [0]

misunderstood bryner... here's the final patch with a new event msg in nsGUIEvent...

Having a message constant means that instead of adding to the XXX comment in nsDOMEvent::GetEventName, you should fix nsDOMEvent::SetEventType.

Oh, and actually, the GetEventName change should use the message constant, not the struct constant. (Yes, the event system sucks. We really need to clean this up.)

Comment on attachment 192526 [details] [diff] [review] final patch >@@ -922,24 +922,26 @@ const char* nsDOMEvent::GetEventName(PRU > case NS_UI_FOCUSIN: > return sEventNames[eDOMEvents_DOMFocusIn]; > case NS_UI_FOCUSOUT: > return sEventNames[eDOMEvents_DOMFocusOut]; > case NS_PAGE_SHOW: > return sEventNames[eDOMEvents_pageshow]; > case NS_PAGE_HIDE: > return sEventNames[eDOMEvents_pagehide]; >+ case NS_XULOVERLAYMERGED_EVENT: >+ return sEventNames[eDOMEvents_XULOverlayMerged]; This should be NS_XULOVERLAYMERGED, since it refers to the event name, not the struct type. >--- widget/public/nsGUIEvent.h 22 Jun 2005 01:53:58 -0000 3.121 >+++ widget/public/nsGUIEvent.h 12 Aug 2005 18:10:13 -0000 >@@ -270,16 +271,17 @@ class nsIURI; > #define NS_XUL_POPUP_SHOWING (NS_XUL_EVENT_START) > #define NS_XUL_POPUP_SHOWN (NS_XUL_EVENT_START+1) > #define NS_XUL_POPUP_HIDING (NS_XUL_EVENT_START+2) > #define NS_XUL_POPUP_HIDDEN (NS_XUL_EVENT_START+3) > #define NS_XUL_COMMAND (NS_XUL_EVENT_START+4) > #define NS_XUL_BROADCAST (NS_XUL_EVENT_START+5) > #define NS_XUL_COMMAND_UPDATE (NS_XUL_EVENT_START+6) > #define NS_XUL_CLICK (NS_XUL_EVENT_START+7) >+#define NS_XUL_OVERLAYMERGED (NS_XUL_EVENT_START+8) Generally the messages are grouped together by the eventStructType that they use. I'd probably do something like this: // XULOverlayMerged events #define NS_XUL_OVERLAYMERGED_START 2800 #define NS_XUL_OVERLAYMERGED (NS_XUL_OVERLAYMERGED_START) It's up to you though; if you leave it like it is I'd at least add a comment to say that this message uses nsXULOverlayMergedEvent. One other question in general, is it possible for content to trick the chrome into doing something undesirable by firing an overlay merged event? Would it make sense to check event.isTrusted? r=me with those addressed.

The DOMClassInfo ordering actually looks like it's going to break stuff there, for two reasons: * one, you should be adding before foreignObject, not after it, since foreignObject is currently disabled. There really needs to be a big "ADD NEW ENTRIES HERE" comment there -- there used to be. * two, your change do sClassInfoData in nsDOMClassInfo.cpp needs to match the order of the array -- otherwise it breaks everything in between. (And, for maintainability, the DOM_CLASSINFO_MAP_* stuff should probably match as well.) Also, you should undo the comment change in nsDOMEvent.cpp -- it's no longer needed.

Comment on attachment 192531 [details] [diff] [review] patch for checkin, all comments addressed. No, nevermind.

(In reply to comment #44) > These operations use standard DOM APIs to work. No, they don't. They use nsIContent APIs, which are not quite the same thing, though they do trigger mutation events. And I wouldn't be surprised if the relevant code dies a grisly death if said mutation events do something like removing the node that's being added by the overlay merge from the DOM.

Comment on attachment 193592 [details] [diff] [review] minimal interface documentation for 1.5, announcing instability requesting approval so I can get this off my plate

That patch doesn't really address issues 1, 2, or 3 from comment 12. 3 is a memory leak, 1 is a crash waiting to happen, 2 seems to have changed from a crash to "just" a random assert in the wrong place in the code with the patch in bug 293460. I appreciate your desire to get this off your plate, but fixing just those three issues should really not be that hard... (as compared to redoing the whole architecture here, which is something I've never asked for, I should point out).

Comment on attachment 193592 [details] [diff] [review] minimal interface documentation for 1.5, announcing instability a=cbeard, please go ahead and land on the branch. need further investigation to everything that dynamic overlays breaks so that we can ensure we address them all.

(see bz's comments above about what we should probably look to get fixed for 1.5 independent of holistic dynamic overlays fix)

Landed the documentation, will continue the patch for 2.0. Resetting flags to get off the blocking lists.

I really do think we should address those three issues for 1.8...

we're looking at the critical bugs for 1.5 and mscott is gonna try to help us here

I return to thinking the easiest way to address 3 may be to make the event patch work properly again. I will look at that tomorrow.

(the problem with the latest event patch is that once I made the event dispatch more "conventional" per dbaron, the event actually seemed to stop firing -- I have not yet figured out why.)

(the problem with the latest event patch is that once I made the event dispatch more "conventional" per dbaron, the event actually seemed to stop firing -- I have not yet figured out why.)

I'd say just switch it back -- it requires a whole bunch of goop in nsEventListenerManager.h and .cpp that you also haven't included, and I'm a little skeptical of whether we should be including all of that for every event we create. But you should probably ask bryner and then listen to him rather than me.

This patch attempts to address issues #1 and #2 from comment 12. 1) Add documentation to the IDL file stating that aObserver must be a valid, non null parameter. 2) Assert in LoadOverlay if the observer is null. (We could optionally now remove the assertion in nsXULDocument::ResumeWalk that asserts if the observer is null). 3) Return NS_ERROR_OUT_OF_MEMORY if we get an error when trying to initialize the hash tables.

> 1) Add documentation to the IDL file stating that aObserver must be a valid, > non null parameter. What if I just want to load an overlay and don't care when it's done loading? That is, I think it makes more sense to not require an observer here and just take out the asserts (but leave the null-check that was added).

Makes it valid to pass in a null observer to LoadOverlay. Remove the assertions complaining about a null observer, leaving in the if (obs) check.

Comment on attachment 194271 [details] [diff] [review] updated patch r+sr=bzbarsky assuming the indent on the things that used to be "else" bodies is fixed and that a comment is added to the idl saying that the observer may be null.

David, Brian agreed with you that the method I was using previously was a little odd for C++, but didn't think it was a problem.

Test cases to come.

carrying forward bz's r/sr. Fixes some white space issues and adds a comment to the idl file. This is the patch I'll be checking into the branch and trunk.

Comment on attachment 194353 [details] [diff] [review] [fixed on trunk and branch] updated patch to be checked in Triage team, this is a reasonably low risk patch for the branch which addresses several issues bz raised about dynamic overlays.

Comment on attachment 194353 [details] [diff] [review] [fixed on trunk and branch] updated patch to be checked in We've now addressed issue #1 and #2 that bz raised in comment 12 on the trunk and the 1.8 branch. It sounds like we are tackling issue #3 on the trunk only and Ben has already started to put together a 1.9 patch for that issue. In short, I think this bug is done for 1.8.

> It sounds like we are tackling issue #3 on the trunk only Why? Have we decided that the leak is just not worth worrying about or something?

Because the patch to address #3 is listed as being for 1.9.

#3 was a request to break the reference cycle through the observer even if the overlay load fails. It has nothing to do with the "1.9" patch except the fact that this patch makes it unnecessary to break said cycle by eliminating the observer altogether.

Are we done here for 1.5 or is there still more to do?

That depends on whether we plan to actually fix the leak that happens on dynamic overlay load cancel/failure or not.

mlk keyword

If LoadOverlayInternal returns an error, then release the overlay load observer for that overlay. There's still the potential for the asynhronous part of the overlay load to throw an error at some point and that will still leak (i.e. errors in ResumeWalk and other places). I didn't see an easy / low risk way to address that. I think Ben's suggested event changes are the best way to address that issue, and we won't be taking that for 1.8b5.

Comment on attachment 197623 [details] [diff] [review] [fixed on trunk and branch]release the observer if LoadOverlayInternal returns an error This is good, but I did mean the overlay load failing async in my comments about leaks earlier. I'm not sure why it would be so dangerous to also drop the listener in OnStopRequest if status is failure... unless we actually end up notifying in that case already? I must admit that it's been a while since I sorted through this ResumeWalk stuff last (when I initially commented on this bug).

(In reply to comment #94) > (From update of attachment 197623 [details] [diff] [review] [edit]) > I'm not sure why it would be so dangerous to also drop > the listener in OnStopRequest if status is failure... unless we actually end up > notifying in that case already? I must admit that it's been a while since I > sorted through this ResumeWalk stuff last (when I initially commented on this > bug). From what I can see, nsXULDocument::ParserObserver::OnStopRequest always calls ResumeWalk, even when the asynchronous overlay load fails. And ResumeWalk always issues the notifications unless it throws an error. Which brings us back to ResumeWalk as the only remaining point which could generate an error which would keep us from issuing the notification. I thought it would be too dangerous / complicated to start messing around with all of the return points in that method (of which there are many), dropping the observer at each one of those points. My two cents...

Yeah, if we're always calling ResumeWalk on load errors, things should be ok enough.

Comment on attachment 197623 [details] [diff] [review] [fixed on trunk and branch]release the observer if LoadOverlayInternal returns an error Low risk change to prevent a potential memory leak in case of an error. This last patch allows us to take this bug off the 1.8b5 radar.

Comment on attachment 197623 [details] [diff] [review] [fixed on trunk and branch]release the observer if LoadOverlayInternal returns an error ok, this patch is now on the 1.8 branch.

Ok, this finishes up the three issues bz raised for 1.8. The remaining work for this bug is intended for the trunk only and is something Ben is working on. I'm removing the blocking+ flag from this bug.

This is not allowed in remote XUL, but the error returned if it's not is not a security error. try{ document.loadOverlay("myOverlay.xul",obs); } catch(e){ alert("error: "+e); } That should *at least* trigger the alert(), but it doesn't. Without the try-catch block the error is: "Error: uncaught exception: [Exception... "Component returned failure code: 0x804b000a [nsIDOMXULDocument.loadOverlay]" nsresult: "0x804b000a (<unknown>)" location: "JS frame :: http://remote/server/index.xul :: loadDynOverlay :: line 72" data: no]"

loadOverlay() doesn't allow relative URIs (and should probably either document that, or better yet fix it).

Has anyone done any testing with loading multiple overlays one after the other? First I looped through a set of overlays, but consistently nothing loaded. Then I just did explicit subsequent calls: document.loadOverlay("a.xul", null); document.loadOverlay("b.xul", null); document.loadOverlay("c.xul", null); Im my tests, only the last call works, and only after the second document load. I'm using Firefox 1.5 RC2/Linux.

This caused bug 319463.

Is this bug now WONTFIX?

Yeah.