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.

copy testcase from duplicate bug #412286

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>

first testcase was a wrong file. override

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.

updated patch: removes an unused test

minor modification of alignement in if/else

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

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 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 on attachment 298115 [details] [diff] [review] patch v1.2 Assuming sicking agrees with my comments, I'd like to see an updated patch.

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

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

carrying over r+

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

carrying over sr+

Comment on attachment 299852 [details] [diff] [review] patch v2.2 a1.9+=damons

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

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.

Closing this again in favor of bug 414747.

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

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

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)

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?

what do you mean by "execute user scripts" ?

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?