391002 - broadcaster/command element failed to re-forward all attributes to the target element

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Bo Yang]

I list the code which cause the problem:
In my XUL file:
<command id="command1" label="label1" oncommand="dosomething();"/>
<command id="command2" label="label2" oncommand="dosomethingelse();"/>

<button id="button1" command="command1"/>

And in my JS file:
function dosomething(){
    var ele = this.getElementById("button1");
    ele.setAttribute("command","command2");
    //Here, reassign the button's command attribute
}

When the above function is called, the button's command handler will be dosomethingelse(). But the label attribute will be unchanged. So are the other attributes if there are. This means that the command element does not forward all attributes to the button. So does the broadcaster element and observes attribute.

Maybe the command/broadcaster element are not designed to be used in such a way, but it is a legal using case, I think.

Comment 2

[arno renevier]

Attachment: testcase: oncommand is updated (alert('cmd2')) but label is not (cmd1)

copy testcase from duplicate bug #412286

Comment 3

[arno renevier]

Comment on attachment 297634 [details]
testcase: oncommand is updated (alert('cmd2')) but label is not (cmd1)

<?xml version="1.0"?>
<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>

<window id="bug412286"
    title="bug412286"
    onload="document.getElementById('btn').setAttribute('command', 'cmd2')"
    xmlns  = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

    <description>after load, button label should be cmd2</description>

  <button id="btn" command="cmd1"/>

    <command id="cmd1" label="cmd1" oncommand="alert('cmd1')"/>
    <command id="cmd2" label="cmd2" oncommand="alert('cmd2')"/>

</window>

Comment 4

[arno renevier]

Attachment: testcase v2

first testcase was a wrong file. override

Comment 5

[arno renevier]

Attachment: patch v1

That patch works by calling AddBroadcasterListenerFor from nsXULDocument::AttributeChanged if needed to resynchronize broadcaster and listener, and by unsetting "synchronized arguments" when broadcaster is removed (I needed to modify XULDocument::SynchronizeBroadcastListe content *and* signature for that).

It also calls RemoveBroadcastListenerFor in nsXULDocument::BeforeSetAttr. That's useful when cmd1 and cmd2 don't have the same arguments: so, it removes arguments related to cmd1 before setting those related to cmd2.

In ordrer for attribute modifications to be redrawn, it calls SetAttr with notify parameter set to true in nsXULDocument::SynchronizeBroadcastListener. May be there are performance issues, but I don't known how to detect and measure them.

There are still a few issues unresolved (such as removeChild'ing a <command/> element), but I think broadcaster behaviour is more consistent with that modification.

Comment 6

[arno renevier]

Attachment: patch v1.1

updated patch: removes an unused test

Comment 7

[arno renevier]

Attachment: patch v1.2

minor modification of alignement in if/else

Comment 8

[Boris Zbarsky [:bzbarsky]]

Neil, you're probably the right person to review this...

Comment 9

[[e-mail]]

Comment on attachment 298115 [details] [diff] [review]
patch v1.2

>+                nsCOMPtr<nsIDOMXULDocument> xuldoc = do_QueryInterface(doc);
>+                xuldoc->RemoveBroadcastListenerFor(oldBroadcaster, this,
>+                                                NS_LITERAL_STRING("*"));
You don't null-check xuldoc; by comparison the existing UnsetAttr code does. Maybe the common code should be factored out into a separate method.

>-            nsAutoString value;
>-            broadcaster->GetAttr(nameSpaceID, name, value);
>-            listener->SetAttr(nameSpaceID, name, attrName->GetPrefix(), value,
>-                              PR_FALSE);
>+            if (synchronize) {
>+                nsAutoString value;
>+                broadcaster->GetAttr(nameSpaceID, name, value);
>+                listener->SetAttr(nameSpaceID, name, attrName->GetPrefix(), 
>+                                    value, PR_TRUE);
>+            } else {
>+                listener->UnsetAttr(nameSpaceID, name, PR_TRUE);
>+            }
I don't think we should be removing attributes if you remove a broadcaster hookup. Also the issue of notifications is thorny - apparently we shouldn't notify until after initial reflow.

>@@ -969,16 +975,41 @@ nsXULDocument::AttributeChanged(nsIDocum
>+    // modifications in broadcasters
Given that you've already removed any previous broadcaster hookup it seems as if all you need to do here is to call CheckBroadcasterHookup. Your code does not for example handle keys and menuitems correctly.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 298115 [details] [diff] [review]
patch v1.2

Neil seems to have started review on this. I wouldn't mind sr'ing though

Comment 11

[[e-mail]]

Comment on attachment 298115 [details] [diff] [review]
patch v1.2

Assuming sicking agrees with my comments, I'd like to see an updated patch.

Comment 12

[arno renevier]

I've begun working on an updated patch. Hopefully, I'll provide a revised version in 2 or 3 days.

Comment 13

[arno renevier]

Attachment: patch v2.0

> I don't think we should be removing attributes if you remove a broadcaster
> hookup.

I did that because of situations such as:

<button id="item" command="cmd1"/>
<commandset id="commandset">
<command id="cmd1" label="cmd1" style="color:blue" oncommand="dump('executing cmd1\n')"/>
<command id="cmd2" label="cmd2" oncommand="dump('executing cmd2\n')"/>
</commandset>

after setting item command to cmd2, style attribute would not be deleted, and button would stay blue.
Also, in

  <button id="item" label="cmd" observes="broadcaster" oncommand="dump('executing cmd')"/>
  <broadcasterset>
  <broadcaster id="broadcaster" accesskey="c"/>
  /broadcasterset>

if you remove item observes attribute, accesskey would still be active.

but may be unsetting attributes is not the right way to go about those situations. Anyway, I think they are quite rare. I will provide a patch that does not unset attributes if you prefer.

> Also the issue of notifications is thorny - apparently we shouldn't
> notify until after initial reflow.

So, now, I set notify argument (when calling SetAttr or UnSetAttr), to mInitialLayoutComplete value, because it is set to true after StartLayout. I don't known if there is a better way.

my patch also adresses other issues you explained in you comment.

Comment 14

[[e-mail]]

Comment on attachment 299212 [details] [diff] [review]
patch v2.0

If I've understood this patch correctly, it might actually fix a bug in menulist.xml - currently we manually copy the four broadcast attributes because the current code doesn't do that correctly, but your code would fix that, at least for the XULDocument case. (The DOMAttrModified still needs it of course.)

>+                                            PRBool synchronize)
I don't like the name of this parameter. It's basically an add/remove flag isn't it? Also it should begin with an a, e.g. aAddingListener

>+    // checks for modifications in broadcasters
>+    PRBool listener, resolved;
>+    rv = CheckBroadcasterHookup(aElement, &listener, &resolved);
>+    if (NS_FAILED(rv)) return;
I don't think it would be a good idea to return at this point.
As far as I can see you might as well just ignore any errors.

Comment 15

[arno renevier]

Attachment: patch v2.1

carrying over r+

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 299836 [details] [diff] [review]
patch v2.1

Looks good, but here's a couple of style nits:

- In nsXULElement::AfterSetAttr():

         if (doc && (aName == nsGkAtoms::observes ||
                           aName == nsGkAtoms::command)) {
-            nsCOMPtr<nsIDOMXULDocument> xuldoc = do_QueryInterface(doc);
[...]
+              RemoveBroadcaster(oldValue);

Fix the indentation (4 spaces)

- In nsXULDocument::SynchronizeBroadcastListener():

+                listener->SetAttr(nameSpaceID, name, attrName->GetPrefix(), 
+                                    value, mInitialLayoutComplete);

Fix second-line indentation.

sr=jst

Comment 17

[arno renevier]

Attachment: patch v2.2

carrying over sr+

Comment 18

[Damon Sicore (:damons)]

Comment on attachment 299852 [details] [diff] [review]
patch v2.2

a1.9+=damons

Comment 19

[Reed Loden [:reed]]

Checking in content/xul/content/src/nsXULElement.cpp;
/cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v  <--  nsXULElement.cpp
new revision: 1.749; previous revision: 1.748
done
Checking in content/xul/content/src/nsXULElement.h;
/cvsroot/mozilla/content/xul/content/src/nsXULElement.h,v  <--  nsXULElement.h
new revision: 1.267; previous revision: 1.266
done
Checking in content/xul/document/src/nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <--  nsXULDocument.cpp
new revision: 1.798; previous revision: 1.797
done
Checking in content/xul/document/src/nsXULDocument.h;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.h,v  <--  nsXULDocument.h
new revision: 1.203; previous revision: 1.202
done
Checking in content/xul/document/test/Makefile.in;
/cvsroot/mozilla/content/xul/document/test/Makefile.in,v  <--  Makefile.in
new revision: 1.5; previous revision: 1.4
done
RCS file: /cvsroot/mozilla/content/xul/document/test/test_bug391002.xul,v
done
Checking in content/xul/document/test/test_bug391002.xul;
/cvsroot/mozilla/content/xul/document/test/test_bug391002.xul,v  <--  test_bug391002.xul
initial revision: 1.1
done

Comment 20

[Steve England [:stevee]]

This bug appears to have caused bug 414747 (random addon-related crashing) and bug 414846 (menu corruption) so I am reopening it as per bug 414747 comment 10.

Comment 21

[Reed Loden [:reed]]

Closing this again in favor of bug 414747.

Comment 22

[Mike Schroepfer]

So given the regressions how bout we back-out for Beta3?

Comment 23

[pal-moz]

(In reply to comment #20)
> This bug appears to have caused bug 414747 (random addon-related crashing) and
> bug 414846 (menu corruption) so I am reopening it as per bug 414747 comment 10.
> 

maybe bug 414907 too.

Comment 24

[Jochen]

Could it be that the checkin caused bug 414846, bug 414907 and others?:

<broadcaster id="broadCaster" someAttr="something" />
<element observes="broadCaster" someAttr="somethingelse" />

As far as I can see, doubling the attribute is enough to break
lots of things. (including customization)

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So you are aware that calling SetAttr with aNotify set to true can execute user scripts right? Are you sure you are doing this at a time when it's safe?

Comment 27

[arno renevier]

what do you mean by "execute user
scripts" ?

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

by "user script" i mean script coming from the webpages. Such scripts might intentionally try to do things like will cause our code to crash, since often such crashes are exploitable.

So the question is, will the code calling SetAttr expect that world changes before SetAttr returns (i.e. documents being deleted, dom trees changing, etc) without crashing?