383890 - crash creating new dom attribute with prefix [@inDOMView::GetChildNodesFor]

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[:Mook]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070609 IceWeasel/3.0a6pre Mook/Wine

MOZ_CO_DATE: 2007-06-09 12:23 -0700

Steps to reproduce:
1. Inspect a XML document, such as data:text/xml,<r/>
2. Select the document node
3. In the DOM Node view, right click and Insert an attribute
4. Node name: foo:bar
   Node value: baz
   Namespace URI: XHTML
5. Click OK

Now building a debug build to get useful info, but it looks like the first arg is null.

Comment 1

[Alex Vincent [:WeirdAl]]

I'll bet this is a DOM crash.

Comment 2

[:Mook]

Attachment: stack at crash

in inDOMView::AttributeChanged, domAttr is null.

At el->GetAttributeNode(attrStr, getter_AddRefs(domAttr));
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/inspector/src/inDOMView.cpp&rev=1.48&mark=720#715
attrStr is "bar", but as far as I can tell GetAttributeNode expects a qualified name ("foo:bar"), hence it returns null.  I guess it's ignoring aNameSpaceID completely?  Not sure on that...

Comment 3

[Alex Vincent [:WeirdAl]]

Okay, I was wrong. :)

sdwilsh, do you want this one, or should I try taking it?

Comment 4

[Shawn Wilsher :sdwilsh]

I'm really busy, so if you can take it, please do!

Comment 5

[:Mook]

Attachment: patch

I am totally unfamiliar with this area of code, so... I'm probably doing it wrong.

It seems to fix the crash (NS_ENSURE_ARG should make sure of that part), and it also seems to show sensible-looking things in the treeview that normally shows attributes.

The delete part is still broken, I think.

There's probably some way to do things without touching the namespace manager, but...

Comment 6

[Shawn Wilsher :sdwilsh]

So after applying this patch and trying the steps to reproduce, I get the following two errors in the error console:

Error: this.mDoc has no properties
Source File: chrome://inspector/content/viewers/domNode/domNodeDialog.js
Line: 123
Error: this.subject.setAttributeNS is not a function
Source File: chrome://inspector/content/viewers/domNode/domNode.js
Line: 386

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 267840 [details] [diff] [review]
patch

general nit: line wrapping at 80 chars

>+    el->GetAttributeNodeNS(attrNS, attrStr, getter_AddRefs(domAttr));
>+    el->GetAttributeNode(attrStr, getter_AddRefs(domAttr));

nit: can you please cast the returned results of these to void to explicitly state that you don't care what they return please.  ex:
(void)el->GetAttributeNode(attrStr, getter_AddRefs(domAttr));

Good coding practices are nice!

r=me with these fixed.

Comment 8

[:Mook]

Attachment: fixed review comments (added cast to void)

Added cast to void; requesting sr.
(Sorry Neil, it looks like I had been using the wrong address for review requests before...)

Comment 9

[[e-mail]]

Comment on attachment 267903 [details] [diff] [review]
fixed review comments (added cast to void)

>+  nsString attrNS;
>+  nsresult rv;
This is C++, not C; please declare these where you use them. sr=me with this fixed.

Comment 10

[:Mook]

Attachment: addresses Neil's sr comments

carrying forward r/sr, ready for checkin.

Thanks everyone :)

Comment 11

[Shawn Wilsher :sdwilsh]

Checking in layout/inspector/src/inDOMView.cpp;
new revision: 1.49; previous revision: 1.48

Comment 12

[[e-mail]]

Comment on attachment 268034 [details] [diff] [review]
addresses Neil's sr comments

>+    nsCOMPtr<nsINameSpaceManager> nsm =
>+      do_GetService(NS_NAMESPACEMANAGER_CONTRACTID);
>+    if (!nsm) {
I only hit this because I was running an old build, but it turns out we have nsContentUtils::NameSpaceManager() which always succeeds.