Bug 5875 – WebCore+SVG fails to report errors, fails to load external scripts

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

WebCore+SVG fails to report errors, fails to load external scripts

This patch fixes both (simple) issues.

[Eric Seidel (no email)]

Created attachment 4845 [details]
Fixes reporting errors, loading external scripts

[Eric Seidel (no email)]

Comment on attachment 4845 [details]
Fixes reporting errors, loading external scripts

Simple patch.  Also does a bit of cleanup to the <script> handling code in
xml_tokenizer.cpp

[Darin Adler]

Comment on attachment 4845 [details]
Fixes reporting errors, loading external scripts

There's no need to call doc->removeChild(root). Putting a child in a new parent
with appendChild automatically removes it from its old location.

This line:

    body->appendChild(root,exceptioncode)

is missing a space after a comma.

The new code and some of the existing code in this function is just working by
"luck" because it doesn't ref the newly created nodes even though in some cases
it makes calls to set them up before calling appendChild to put them into the
tree. Those calls are allowed to ref/deref the node, which would then be
deleted. One way to fix this is to do always do the appendChild first. Another
alternative is to use SharedPtr for some of the local variables. A third is to
add explicit ref/deref calls.

The name "newRoot" is a confusing name for the local variable to hold the newly
created HTML element, since "root" is used as the name for a body element and
indeed "root" gets set to "body" not "newRoot".

The code in XMLTokenizer::executeScripts seems like it belongs in the script
element classes themselves somehow rather than here in the tokenizer. If there
is some common base class for HTML and SVG script elements then they could have
a virtual function that returns the script's source URL. It seems that there's
logic that would need to be shared between HTML and SVG script elements to
support dynamic loading of scripts. It also seems not so good have the xlink
and href attribute names as string literals here, meaning atomic strings will
be created each time this code is run.

[Eric Seidel (no email)]

Created attachment 4868 [details]
Addresses Darin's comments (adds XLinkNames)

[Eric Seidel (no email)]

Created attachment 4869 [details]
Addresses Darin's comments (XLinkNames broken out)

[Eric Seidel (no email)]

Comment on attachment 4869 [details]
Addresses Darin's comments (XLinkNames broken out)

I addressed Darin's concerns regarding making the xlink qualified name (now
depends on 5887) as well as using SharedPtr to make sure that the ElementImpls
we create don't disappear out from underneath us.  After discussing with mjs
and darin, I did not attempt to abstract the ScriptElement logic into a common
baseclass for SVG & HTML at this time.	We may still chose to do so in the
future, but doing so is not critical at this moment.

Either Darin our Maciej should be able to review this.

[Darin Adler]

Comment on attachment 4869 [details]
Addresses Darin's comments (XLinkNames broken out)

The name xhtmlParseErrorHeader makes it sound like "parse" is a verb, so I
think it should be improved.

The "h3" and "fixed" are both appended immediately after being created, so the
SharedPtr is not needed in those cases.

Do we have layout tests that test these parse error cases?

r=me

[Eric Seidel (no email)]

(In reply to comment #7)
> (From update of attachment 4869 [details] [edit])
> The name xhtmlParseErrorHeader makes it sound like "parse" is a verb, so I
> think it should be improved.

I changed it to createXHTMLParserErrorHeader.

> The "h3" and "fixed" are both appended immediately after being created, so the
> SharedPtr is not needed in those cases.

I left these as is, I felt that the ref count churn was OK, and made for cleaner (more defensive) coding, if  
someone else were to change that code, or the append were to fail (and not release the child), etc.

> Do we have layout tests that test these parse error cases?

I added 7 layout tests:
2 to cover xml parse errors
3 to cover xslt related errors
and 2 more to cover svg parse errors.