282103 - Dynamic Overlays

Status: RESOLVED WONTFIX

(Core :: XUL, defect, P1)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

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.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

Comment 2

[Robert Accettura [:raccettura]]

(In reply to comment #0)
> Support loading of XUL overlays after the document has been displayed 

Will this allow for installing extensions without restarting?

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

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 4

[Brian Ryner (not reading)]

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 5

[timeless]

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 6

[Brian Ryner (not reading)]

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 7

[Johnny Stenback (:jst)]

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

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: newer patch

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

(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 10

[Brian Ryner (not reading)]

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 11

[Johnny Stenback (:jst)]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[timeless]

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

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

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. 

Comment 15

[Boris Zbarsky [:bzbarsky]]

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?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Note: this may have caused crash bug 284330

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Ben Goodger (use ben at mozilla dot org for email)]

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? 

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

(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).

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[Scott MacGregor]

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.

Comment 22

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: initial prototype - not done yet.

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.

Comment 23

[Scott MacGregor]

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. 

Comment 24

[Scott MacGregor]

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.

Comment 25

[Scott MacGregor]

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.

Comment 26

[Scott MacGregor]

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. 

Comment 27

[Scott MacGregor]

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. 

Comment 28

[Mike Kaply [:mkaply]]

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?

Comment 29

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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.

Comment 30

[Ben Goodger (use ben at mozilla dot org for email)]

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

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.

Comment 32

[Ben Goodger (use ben at mozilla dot org for email)]

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

Comment 33

[Ben Goodger (use ben at mozilla dot org for email)]

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

Comment 34

[Boris Zbarsky [:bzbarsky]]

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.

Comment 35

[Ben Goodger (use ben at mozilla dot org for email)]

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

Comment 36

[Ben Goodger (use ben at mozilla dot org for email)]

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. 

Comment 37

[Boris Zbarsky [:bzbarsky]]

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?

Comment 38

[Boris Zbarsky [:bzbarsky]]

Looks like this also caused bug 293460.

Comment 39

[Asa Dotzler [:asa]]

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

Comment 40

[Ben Goodger (use ben at mozilla dot org for email)]

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

Comment 41

[Ben Goodger (use ben at mozilla dot org for email)]

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

Comment 42

[Mike Schroepfer]

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

Comment 43

[Ben Goodger (use ben at mozilla dot org for email)]

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)

Comment 44

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: updated to trunk patch

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.

Comment 45

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: updated.

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 47

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 48

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 49

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You should rev the UUID of nsIDOMXULDocument.

Comment 51

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: address dbaron/vlad review comments

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.

Comment 52

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

rev uuid of nsIDOMXULDocument

Comment 53

[Ben Goodger (use ben at mozilla dot org for email)]

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 54

[Johnny Stenback (:jst)]

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.

Comment 55

[Ben Goodger (use ben at mozilla dot org for email)]

(In reply to comment #54)
> +	 for (i = 0; i < count; ++i)
> +	     FireXULOverlayMergedEvent(mPendingOverlayLoadNotifications[0]);
> +		mPendingOverlayLoadNotifications.Clear();

I am a moron :-)

-Ben

Comment 56

[Brian Ryner (not reading)]

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.

Comment 57

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

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

Comment 58

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: final patch

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

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 60

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 61

[Brian Ryner (not reading)]

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.

Comment 62

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch for checkin, all comments addressed.

Comment 63

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 64

[Ben Goodger (use ben at mozilla dot org for email)]

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

No, nevermind.

Comment 65

[Boris Zbarsky [:bzbarsky]]

(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 66

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: minimal interface documentation for 1.5, announcing instability

Comment 67

[Ben Goodger (use ben at mozilla dot org for email)]

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

Comment 68

[Boris Zbarsky [:bzbarsky]]

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 69

[Chris Beard]

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.

Comment 70

[Chris Beard]

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

Comment 71

[Ben Goodger (use ben at mozilla dot org for email)]

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

Comment 72

[Boris Zbarsky [:bzbarsky]]

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

Comment 73

[Asa Dotzler [:asa]]

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

Comment 74

[Ben Goodger (use ben at mozilla dot org for email)]

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. 

Comment 75

[Ben Goodger (use ben at mozilla dot org for email)]

(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.)

Comment 76

[Ben Goodger (use ben at mozilla dot org for email)]

(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.)

Comment 77

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 78

[Scott MacGregor]

Attachment: patch to address a couple of bz comments

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.

Comment 79

[Boris Zbarsky [:bzbarsky]]

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

Comment 80

[Scott MacGregor]

Attachment: updated patch

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 81

[Boris Zbarsky [:bzbarsky]]

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.

Comment 82

[Ben Goodger (use ben at mozilla dot org for email)]

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. 

Comment 83

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: For 1.9, a patch

Test cases to come.

Comment 84

[Scott MacGregor]

Attachment: [fixed on trunk and branch] updated patch to be checked in

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 85

[Scott MacGregor]

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 86

[Scott MacGregor]

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.

Comment 87

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 88

[Scott MacGregor]

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

Comment 89

[Boris Zbarsky [:bzbarsky]]

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

Comment 90

[Asa Dotzler [:asa]]

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

Comment 91

[Boris Zbarsky [:bzbarsky]]

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

Comment 92

[Worcester12345]

mlk keyword

Comment 93

[Scott MacGregor]

Attachment: [fixed on trunk and branch]release the observer if LoadOverlayInternal returns an error

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 94

[Boris Zbarsky [:bzbarsky]]

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

Comment 95

[Scott MacGregor]

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

Comment 96

[Boris Zbarsky [:bzbarsky]]

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

Comment 97

[Scott MacGregor]

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 98

[Scott MacGregor]

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.

Comment 99

[Scott MacGregor]

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.

Comment 100

[David Palm]

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

Comment 101

[Boris Zbarsky [:bzbarsky]]

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

Comment 102

[Brian King [:kinger]]

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.

Comment 103

[Boris Zbarsky [:bzbarsky]]

This caused bug 319463.

Comment 104

[kernp25]

Is this bug now WONTFIX?

Comment 105

[Brendan Dahl [:bdahl]]

Yeah.