Bug 59313 - ASSERT failure in SVGUseElement
Summary: ASSERT failure in SVGUseElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Leo Yang
URL:
Keywords:
Depends on: 59413
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-24 22:35 PDT by Leo Yang
Modified: 2011-04-26 02:57 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.42 KB, patch)
2011-04-24 23:02 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Revised patch (5.79 KB, patch)
2011-04-25 03:39 PDT, Leo Yang
zimmermann: review+
Details | Formatted Diff | Diff
Patch - test case reworked (6.32 KB, patch)
2011-04-25 23:43 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Revised patch - test case reworked (6.27 KB, patch)
2011-04-26 02:27 PDT, Leo Yang
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Yang 2011-04-24 22:35:56 PDT
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>
Comment 1 Leo Yang 2011-04-24 23:02:12 PDT
Created attachment 90902 [details]
Patch
Comment 2 WebKit Review Bot 2011-04-24 23:24:50 PDT
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
Comment 3 Leo Yang 2011-04-24 23:33:53 PDT
(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.
Comment 4 Nikolas Zimmermann 2011-04-25 01:05:23 PDT
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?
Comment 5 Leo Yang 2011-04-25 01:17:02 PDT
(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()));
Comment 6 Leo Yang 2011-04-25 01:18:10 PDT
(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
Comment 7 Nikolas Zimmermann 2011-04-25 02:03:37 PDT
(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?
Comment 8 Leo Yang 2011-04-25 02:29:20 PDT
(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.
Comment 9 Leo Yang 2011-04-25 02:38:21 PDT
(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.
Comment 10 Nikolas Zimmermann 2011-04-25 03:05:48 PDT
(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!
Comment 11 Leo Yang 2011-04-25 03:39:02 PDT
Created attachment 90912 [details]
Revised patch
Comment 12 WebKit Review Bot 2011-04-25 03:57:36 PDT
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
Comment 13 Nikolas Zimmermann 2011-04-25 08:01:42 PDT
Comment on attachment 90912 [details]
Revised patch

r=me.
Comment 14 Leo Yang 2011-04-25 19:04:36 PDT
Committed r84864: <http://trac.webkit.org/changeset/84864>
Comment 15 WebKit Review Bot 2011-04-25 19:53:12 PDT
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
Comment 16 Eric Seidel (no email) 2011-04-25 19:55:16 PDT
Looks like this broke a test on mac...
Comment 17 Leo Yang 2011-04-25 21:08:49 PDT
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?
Comment 18 Leo Yang 2011-04-25 23:43:16 PDT
Created attachment 91070 [details]
Patch - test case reworked
Comment 19 Nikolas Zimmermann 2011-04-26 02:06:39 PDT
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?
Comment 20 Leo Yang 2011-04-26 02:22:54 PDT
(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.
Comment 21 Leo Yang 2011-04-26 02:27:18 PDT
Created attachment 91084 [details]
Revised patch - test case reworked
Comment 22 Nikolas Zimmermann 2011-04-26 02:49:49 PDT
Comment on attachment 91084 [details]
Revised patch - test case reworked

r=me.
Comment 23 Leo Yang 2011-04-26 02:57:38 PDT
Committed r84899: <http://trac.webkit.org/changeset/84899>