ASSERT failure in SVGUseElement

• Component: SVG
• Status: RESOLVED FIXED

[Leo Yang]

The following non-wellformed document trigger 'ASSERT(!m_isPendingResource)' in SVGUseElement::insertedIntoDocument()

<?xml version="1.0" standalone="no"?>
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.0" width="400" height="400">
    <use xlink:href="#undefined">
</svg>

[Leo Yang]

Created attachment 90902 [details]
Patch

[WebKit Review Bot]

Attachment 90902 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8507204
Unexpected failures:
svg/custom/use-crash-in-non-wellformed-document.svg

[Leo Yang]

(In reply to comment #2)
> Attachment 90902 [details] did not pass chromium-ews:
> Output: http://queues.webkit.org/results/8507204
> Unexpected failures:
> svg/custom/use-crash-in-non-wellformed-document.svg

Something wrong with chromium-linux bot? There were many patches didn't pass tests.

[Nikolas Zimmermann]

Comment on attachment 90902 [details]
Patch

Can we rewrite this:
if (document()->isSVGDocument() ....
    ASSERT(m_targetElementInstance || m_targetElementInstance);
else
   ....

This might make more sense, eh?

[Leo Yang]

(In reply to comment #4)
> (From update of attachment 90902 [details])
> Can we rewrite this:
> if (document()->isSVGDocument() ....
>     ASSERT(m_targetElementInstance || m_targetElementInstance);
> else
>    ....
> 
> This might make more sense, eh?

hmm, how about this one?

ASSERT((!m_targetElementInstance && !m_targetElementInstance) || ((document()->isSVGDocument() || document()->isXHTMLDocument()) && !static_cast<XMLDocumentParser*>(document()->parser())->wellFormed()));

[Leo Yang]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 90902 [details] [details])
> > Can we rewrite this:
> > if (document()->isSVGDocument() ....
> >     ASSERT(m_targetElementInstance || m_targetElementInstance);
> > else
> >    ....
> > 
> > This might make more sense, eh?
> 
> hmm, how about this one?
> 
> ASSERT((!m_targetElementInstance && !m_targetElementInstance) || ((document()->isSVGDocument() || document()->isXHTMLDocument()) && !static_cast<XMLDocumentParser*>(document()->parser())->wellFormed()));

replacing the second m_targetElementInstance with m_isPendingResource

[Nikolas Zimmermann]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 90902 [details] [details] [details])
> > > Can we rewrite this:
> > > if (document()->isSVGDocument() ....
> > >     ASSERT(m_targetElementInstance || m_targetElementInstance);
> > > else
> > >    ....
> > > 
> > > This might make more sense, eh?
> > 
> > hmm, how about this one?
> > 
> > ASSERT((!m_targetElementInstance && !m_targetElementInstance) || ((document()->isSVGDocument() || document()->isXHTMLDocument()) && !static_cast<XMLDocumentParser*>(document()->parser())->wellFormed()));
> 
> replacing the second m_targetElementInstance with m_isPendingResource

What happens if this assertion would ever fire - you won't go what's going on (is targetElementInstance 0 or isPendingResource etc..) - it's just confusing.
Maybe it would help to add a static inline bool isWellFormedDocument(Document*) helper method, and using the if branch I suggested as well?

[Leo Yang]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (From update of attachment 90902 [details] [details] [details] [details])
> > > > Can we rewrite this:
> > > > if (document()->isSVGDocument() ....
> > > >     ASSERT(m_targetElementInstance || m_targetElementInstance);
> > > > else
> > > >    ....
> > > > 
> > > > This might make more sense, eh?
> > > 
> > > hmm, how about this one?
> > > 
> > > ASSERT((!m_targetElementInstance && !m_targetElementInstance) || ((document()->isSVGDocument() || document()->isXHTMLDocument()) && !static_cast<XMLDocumentParser*>(document()->parser())->wellFormed()));
> > 
> > replacing the second m_targetElementInstance with m_isPendingResource
> 
> What happens if this assertion would ever fire - you won't go what's going on (is targetElementInstance 0 or isPendingResource etc..) - it's just confusing.
> Maybe it would help to add a static inline bool isWellFormedDocument(Document*) helper method, and using the if branch I suggested as well?

inline function is a good idea, so we need ASSERT(document->isSVGDocument() || document->isXHTMLDocument()) before we call static_cast<XMLDocumentParser*>(document->parser())->wellFormed() in isWellFormedDocument()?

release mode also cost a little time of 'if' branch and function call(or inline instructions) of isWellFormedDocument. Is the following ok to you?

ASSERT(!targetElementInstance || !isWellFormedDocument(document());
ASSERT(!m_isPendingResource || !isWellFormedDocument(document());

Although in debug mode we need an extra isWellFormedDocument, in release mode we don't need any extra work.

[Leo Yang]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > (From update of attachment 90902 [details] [details] [details] [details] [details])
> > > > > Can we rewrite this:
> > > > > if (document()->isSVGDocument() ....
> > > > >     ASSERT(m_targetElementInstance || m_targetElementInstance);
> > > > > else
> > > > >    ....
> > > > > 
> > > > > This might make more sense, eh?
> > > > 
> > > > hmm, how about this one?
> > > > 
> > > > ASSERT((!m_targetElementInstance && !m_targetElementInstance) || ((document()->isSVGDocument() || document()->isXHTMLDocument()) && !static_cast<XMLDocumentParser*>(document()->parser())->wellFormed()));
> > > 
> > > replacing the second m_targetElementInstance with m_isPendingResource
> > 
> > What happens if this assertion would ever fire - you won't go what's going on (is targetElementInstance 0 or isPendingResource etc..) - it's just confusing.
> > Maybe it would help to add a static inline bool isWellFormedDocument(Document*) helper method, and using the if branch I suggested as well?
> 
> inline function is a good idea, so we need ASSERT(document->isSVGDocument() || document->isXHTMLDocument()) before we call static_cast<XMLDocumentParser*>(document->parser())->wellFormed() in isWellFormedDocument()?
> 
> release mode also cost a little time of 'if' branch and function call(or inline instructions) of isWellFormedDocument. Is the following ok to you?
> 
> ASSERT(!targetElementInstance || !isWellFormedDocument(document());
> ASSERT(!m_isPendingResource || !isWellFormedDocument(document());
> 
> Although in debug mode we need an extra isWellFormedDocument, in release mode we don't need any extra work.

Actually, a macro like DEBUG_CODE(
// Debug mode code goes here
)
would be the right one we want.

[Nikolas Zimmermann]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > (From update of attachment 90902 [details] [details] [details] [details] [details])
> > > > > Can we rewrite this:
> > > > > if (document()->isSVGDocument() ....
> > > > >     ASSERT(m_targetElementInstance || m_targetElementInstance);
> > > > > else
> > > > >    ....
> > > > > 
> > > > > This might make more sense, eh?
> > > > 
> > > > hmm, how about this one?
> > > > 
> > > > ASSERT((!m_targetElementInstance && !m_targetElementInstance) || ((document()->isSVGDocument() || document()->isXHTMLDocument()) && !static_cast<XMLDocumentParser*>(document()->parser())->wellFormed()));
> > > 
> > > replacing the second m_targetElementInstance with m_isPendingResource
> > 
> > What happens if this assertion would ever fire - you won't go what's going on (is targetElementInstance 0 or isPendingResource etc..) - it's just confusing.
> > Maybe it would help to add a static inline bool isWellFormedDocument(Document*) helper method, and using the if branch I suggested as well?
> 
> inline function is a good idea, so we need ASSERT(document->isSVGDocument() || document->isXHTMLDocument()) before we call static_cast<XMLDocumentParser*>(document->parser())->wellFormed() in isWellFormedDocument()?
> 
> release mode also cost a little time of 'if' branch and function call(or inline instructions) of isWellFormedDocument. Is the following ok to you?
Yeah, you're right. The branch should be avoided.
> 
> ASSERT(!targetElementInstance || !isWellFormedDocument(document());
> ASSERT(!m_isPendingResource || !isWellFormedDocument(document());
Fine with me!

[Leo Yang]

Created attachment 90912 [details]
Revised patch

[WebKit Review Bot]

Attachment 90912 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8504652
Unexpected failures:
svg/custom/use-crash-in-non-wellformed-document.svg

[Nikolas Zimmermann]

Comment on attachment 90912 [details]
Revised patch

r=me.

[Leo Yang]

Committed r84864: <http://trac.webkit.org/changeset/84864>

[WebKit Review Bot]

http://trac.webkit.org/changeset/84864 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
svg/custom/use-crash-in-non-wellformed-document.svg

[Eric Seidel (no email)]

Looks like this broke a test on mac...

[Leo Yang]

The patch has been rolled out.

It seems that the test result is platform dependent? The test is an non-wellformed svg document and the expected result is the error message of the parser. Is the parser message platform dependent? Do we have a good test way for this case? Manually test?

[Leo Yang]

Created attachment 91070 [details]
Patch - test case reworked

[Nikolas Zimmermann]

Comment on attachment 91070 [details]
Patch - test case reworked

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

Quite hacky, but somewhat okay. In the past we used to accept platform dependant results for cases like this. Why did you roll out in first place instead of adding specific results?

> LayoutTests/svg/custom/use-crash-in-non-wellformed-document.svg:18
> +                document.body.offsetWidth;

This is needed as well, you want to force layout here? Why?

[Leo Yang]

(In reply to comment #19)
> (From update of attachment 91070 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91070&action=review
> 
> Quite hacky, but somewhat okay. In the past we used to accept platform dependant results for cases like this. Why did you roll out in first place instead of adding specific results?
I didn't know to rebase test case from the build server :p
is it 'webkit-patch rebaseline' ?

> 
> > LayoutTests/svg/custom/use-crash-in-non-wellformed-document.svg:18
> > +                document.body.offsetWidth;
> 
> This is needed as well, you want to force layout here? Why?

No need actually.

[Leo Yang]

Created attachment 91084 [details]
Revised patch - test case reworked

[Nikolas Zimmermann]

Comment on attachment 91084 [details]
Revised patch - test case reworked

r=me.

[Leo Yang]

Committed r84899: <http://trac.webkit.org/changeset/84899>