Bug 88179 – Hit ASSERT when editing attribute value in Element in SVG Document

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Rob Buis]

To reproduce:

1. load any standalone SVG document
2. start web inspector
3. edit an attribute of an Element

Expected:

attribute should be changed

Actual:

ASSERTION FAILED: !node || node->isHTMLElement()
/Users/rwlbuis/work/WebKit/Source/WebCore/html/HTMLElement.h(151) : WebCore::HTMLElement *WebCore::toHTMLElement(WebCore::Node *)
1   0x10cce2e0d WebCore::toHTMLElement(WebCore::Node*)
2   0x10d6a19c7 WebCore::InspectorDOMAgent::setAttributesAsText(WTF::String*, int, WTF::String const&, WTF::String const*)
3   0x10d6a1ed5 non-virtual thunk to WebCore::InspectorDOMAgent::setAttributesAsText(WTF::String*, int, WTF::String const&, WTF::String const*)
4   0x10d63e7e2 WebCore::InspectorBackendDispatcherImpl::DOM_setAttributesAsText(long, WebCore::InspectorObject*)

[Rob Buis]

Created attachment 145467 [details]
Patch

[Pavel Feldman]

Comment on attachment 145467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145467&action=review

> Source/WebCore/inspector/InspectorDOMAgent.cpp:584
>      toHTMLElement(parsedElement.get())->setInnerHTML("<span " + text + "></span>", ec);

Now that you use createHTMLElement, you don't need this cast.

[Pavel Feldman]

Btw, you probably want a test to go with this change.

[Rob Buis]

Hi,

(In reply to comment #3)
> Btw, you probably want a test to go with this change.

Sorry, I did not see this comment. I'll try to come up with a testcase.
Cheers,

Rob.

[Rob Buis]

Created attachment 145617 [details]
Patch

[Pavel Feldman]

Comment on attachment 145617 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145617&action=review

> LayoutTests/inspector/elements/set-attribute-non-html.svg:104
> +            targetNode.setAttribute("foo", "foo2='baz2' foo3='baz3'");

You only want this test case to test your chanthis 

Btw, this patch is missing the expectations file, r- for this.

[Pavel Feldman]

> You only want this test case to test your chanthis 
> 

*changes

[Rob Buis]

Created attachment 145620 [details]
Patch

[Pavel Feldman]

Comment on attachment 145620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145620&action=review

Thanks for adding the test!

> LayoutTests/inspector/elements/set-attribute-non-html.svg:20
> +                InspectorTest.addResult("=== Set attribute as text ===");

Why is this message not a part of the expectations? Dumping messages and tree below should end up there. It is added via inspector harness that adds divs under the body in the layout test page.

[Rob Buis]

Hi Pavel,

(In reply to comment #9)
> (From update of attachment 145620 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145620&action=review
> 
> Thanks for adding the test!

No problem.

> > LayoutTests/inspector/elements/set-attribute-non-html.svg:20
> > +                InspectorTest.addResult("=== Set attribute as text ===");
> 
> Why is this message not a part of the expectations? Dumping messages and tree below should end up there. It is added via inspector harness that adds divs under the body in the layout test page.

The situation is slightly more complicated :(
The problem only occurs when the document in non HTML, so I took .svg as basis. However the underlying framework overall assumes document = HTML. So in output() document.createElement and document.body will not do the right thing for SVG. So either we need to make the framework take non HTML document into account or we accepts the fact that the testcase does not crash as good enough. What do you think?
Cheers,

Rob.

[Pavel Feldman]

> What do you think?

That sounds Ok. I would remove InspectorTest.addResult .dump* calls then to avoid the confusion.

[Rob Buis]

Created attachment 145624 [details]
Patch

[Pavel Feldman]

Comment on attachment 145624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145624&action=review

> LayoutTests/inspector/elements/set-attribute-non-html.svg:17
> +            targetNode.setAttribute("foo", "foo2='baz2' foo3='baz3'");

You should leave a callback and call next() to complete the suite execution. Otherwise it should time out.

[Rob Buis]

(In reply to comment #13)
> (From update of attachment 145624 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145624&action=review
> 
> > LayoutTests/inspector/elements/set-attribute-non-html.svg:17
> > +            targetNode.setAttribute("foo", "foo2='baz2' foo3='baz3'");
> 
> You should leave a callback and call next() to complete the suite execution. Otherwise it should time out.

Sorry, new to this framework:) Will retry.

[Rob Buis]

Created attachment 145636 [details]
Patch

[WebKit Review Bot]

Comment on attachment 145636 [details]
Patch

Rejecting attachment 145636 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
  return self.open(self.click(*args, **kwds))
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 203, in open
    return self._mech_open(url, data, timeout=timeout)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 255, in _mech_open
    raise response
webkitpy.thirdparty.autoinstalled.mechanize._response.httperror_seek_wrapper: HTTP Error 500: Internal Server Error

Full output: http://queues.webkit.org/results/12897426

[WebKit Review Bot]

Comment on attachment 145636 [details]
Patch

Clearing flags on attachment: 145636

Committed r119483: <http://trac.webkit.org/changeset/119483>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rob Buis]

(In reply to comment #18)
> All reviewed patches have been landed.  Closing bug.

Seems to have worked the second time... Pavel, thanks for the patience and quick reviews!