216721 - Make mAttachedQueue a Stack

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[timeless]

bz asked about this. i wanted to avoid doing it because i was afraid things like
bug 215325 would happen. well they happened anyway. and amusingly enough for the
case in that bug, doing this would 'fix' the problem. i'm not suggesting this
change as a replacement for the change in that bug, but i'd like to make this
change, and we can deal with fallout during 1.6a.

Comment 1

[timeless]

Attachment: make the queue a stack

Comment 2

[[e-mail]]

Comment on attachment 130101 [details] [diff] [review]
make the queue a stack

>-    nsCOMPtr<nsISupports> supp;
>-    mAttachedQueue->GetElementAt(0, getter_AddRefs(supp));
>-    mAttachedQueue->RemoveElementAt(0);
>+  while (NS_SUCCEEDED(mAttachedQueue->Count(&count)) && count--) {
>+    nsCOMPtr<nsISupports> supp = do_QueryElementAt(mAttachedQueue, count);
>+    mAttachedQueue->RemoveElementAt(count);
> 
>     nsCOMPtr<nsIXBLBinding> binding(do_QueryInterface(supp));
>     if (binding)
When I said you should use do_QueryElementAt I meant
nsCOMPtr<nsIXBLBinding> binding(do_QueryElementAt(mAttachedQueue, count));

Comment 3

[[e-mail]]

Anyway, the patch does appear to resolve bug 215325.

Comment 4

[timeless]

Attachment: make the queue a stack and really save a QI

Comment 5

[[e-mail]]

Comment on attachment 130129 [details] [diff] [review]
make the queue a stack and really save a QI

Transferring timeless' review requests to the patch I actually tested.

Comment 6

[Brian Ryner (not reading)]

timeless, how about a little more explanation here?  For example, what problem
does your patch solve?

Comment 7

[timeless]

ok, here's the way this xbl stuff works (I think):

mozilla encounters a <foo> and tries to construct it.
<foo> contains a <bar>, so mozilla considers constructing <bar>.

in the original code it would put <bar> on the stack, call the cleanup function
(which causes recursion/crashes) which cleaned out <bar>.

in the current code it puts <bar> in the queue and finishes off <foo>, albeit
unhappily because it doesn't construct <bar> in time, resulting in bugs like bug
215325.

with the patch we get the old behavior where <foo> pauses, adds <bar> to the
list, and processes <bar>.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 130129 [details] [diff] [review]
make the queue a stack and really save a QI

Looks good, assuming we're all in agreement that this restores the original
firing order of the constructors.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 130129 [details] [diff] [review]
make the queue a stack and really save a QI

sr=bzbarsky if you also rename mAttachedQueue and mProcessningAttachedQueue to
reflect the fact that it's a stack, not a queue.

Comment 10

[timeless]

checked in