Bug 88179 - Hit ASSERT when editing attribute value in Element in SVG Document
Summary: Hit ASSERT when editing attribute value in Element in SVG Document
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-02 18:56 PDT by Rob Buis
Modified: 2012-06-05 04:39 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.31 KB, patch)
2012-06-02 19:09 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.44 KB, patch)
2012-06-04 12:40 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (2.75 KB, patch)
2012-06-04 13:21 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (2.32 KB, patch)
2012-06-04 14:12 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (2.67 KB, patch)
2012-06-04 15:07 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2012-06-02 18:56:15 PDT
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*)
Comment 1 Rob Buis 2012-06-02 19:09:15 PDT
Created attachment 145467 [details]
Patch
Comment 2 Pavel Feldman 2012-06-04 05:02:54 PDT
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.
Comment 3 Pavel Feldman 2012-06-04 05:03:31 PDT
Btw, you probably want a test to go with this change.
Comment 4 Rob Buis 2012-06-04 08:44:43 PDT
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.
Comment 5 Rob Buis 2012-06-04 12:40:37 PDT
Created attachment 145617 [details]
Patch
Comment 6 Pavel Feldman 2012-06-04 12:44:59 PDT
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.
Comment 7 Pavel Feldman 2012-06-04 12:45:57 PDT
> You only want this test case to test your chanthis 
> 

*changes
Comment 8 Rob Buis 2012-06-04 13:21:37 PDT
Created attachment 145620 [details]
Patch
Comment 9 Pavel Feldman 2012-06-04 13:32:13 PDT
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.
Comment 10 Rob Buis 2012-06-04 13:37:44 PDT
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.
Comment 11 Pavel Feldman 2012-06-04 14:01:23 PDT
> What do you think?

That sounds Ok. I would remove InspectorTest.addResult .dump* calls then to avoid the confusion.
Comment 12 Rob Buis 2012-06-04 14:12:11 PDT
Created attachment 145624 [details]
Patch
Comment 13 Pavel Feldman 2012-06-04 14:18:26 PDT
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.
Comment 14 Rob Buis 2012-06-04 14:31:02 PDT
(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.
Comment 15 Rob Buis 2012-06-04 15:07:30 PDT
Created attachment 145636 [details]
Patch
Comment 16 WebKit Review Bot 2012-06-05 00:32:33 PDT
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
Comment 17 WebKit Review Bot 2012-06-05 04:35:48 PDT
Comment on attachment 145636 [details]
Patch

Clearing flags on attachment: 145636

Committed r119483: <http://trac.webkit.org/changeset/119483>
Comment 18 WebKit Review Bot 2012-06-05 04:35:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Rob Buis 2012-06-05 04:39:42 PDT
(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!