Closed Bug 391002 Opened 14 years ago Closed 13 years ago

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

Categories

(Core :: XUL, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: Techrazy.Yang, Assigned: arno)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(2 files, 6 obsolete files)

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.
Duplicate of this bug: 412286
copy testcase from duplicate bug #412286
Keywords: testcase
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>
Attached file testcase v2
first testcase was a wrong file. override
Attachment #297634 - Attachment is obsolete: true
Attached patch patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → arenevier
Status: NEW → ASSIGNED
Attached patch patch v1.1 (obsolete) — Splinter Review
updated patch: removes an unused test
Attachment #298107 - Attachment is obsolete: true
Attached patch patch v1.2 (obsolete) — Splinter Review
minor modification of alignement in if/else
Attachment #298109 - Attachment is obsolete: true
Attachment #298115 - Flags: review?(bzbarsky)
Attachment #298115 - Flags: review?(bzbarsky) → review?(jonas)
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
Attachment #298115 - Flags: review?(jonas) → review?(neil)
Comment on attachment 298115 [details] [diff] [review]
patch v1.2

Assuming sicking agrees with my comments, I'd like to see an updated patch.
Attachment #298115 - Flags: review?(neil) → review-
I've begun working on an updated patch. Hopefully, I'll provide a revised version in 2 or 3 days.
Attached patch patch v2.0 (obsolete) — Splinter Review
> 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.
Attachment #298115 - Attachment is obsolete: true
Attachment #299212 - Flags: review?(neil)
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.
Attachment #299212 - Flags: review?(neil) → review+
Attached patch patch v2.1 (obsolete) — Splinter Review
carrying over r+
Attachment #299212 - Attachment is obsolete: true
Attachment #299836 - Flags: superreview?(jst)
Attachment #299836 - Flags: review+
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
Attachment #299836 - Flags: superreview?(jst) → superreview+
Attached patch patch v2.2Splinter Review
carrying over sr+
Attachment #299836 - Attachment is obsolete: true
Attachment #299852 - Flags: superreview+
Attachment #299852 - Flags: review+
Attachment #299852 - Flags: approval1.9?
Comment on attachment 299852 [details] [diff] [review]
patch v2.2

a1.9+=damons
Attachment #299852 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
Version: unspecified → Trunk
Blocks: 414676
Depends on: 414747
Depends on: 414846
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: blocking1.9?
Target Milestone: mozilla1.9beta3 → mozilla1.9beta4
Closing this again in favor of bug 414747.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9beta4 → mozilla1.9beta3
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)
Duplicate of this bug: 396439
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?
Depends on: 442227
Blocks: 445177
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
No longer blocks: 445177
Depends on: 445177
You need to log in before you can comment on or make changes to this bug.