Bug 143334 – Remove unnecessary ASSERT from XMLDocumentParser::notifyFinished()

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: WebCore Misc.

[Gabor Rapcsanyi]

Remove the ASSERT(m_pendingScript->accessCount() > 0) because if the MemoryCache is disabled then we never icrease the accessCount.

[Gabor Rapcsanyi]

Created attachment 249976 [details]
proposed patch

[Alexey Proskuryakov]

Comment on attachment 249976 [details]
proposed patch

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

> Source/WebCore/ChangeLog:9
> +        Remove the ASSERT(m_pendingScript->accessCount() > 0) because if the MemoryCache is disabled
> +        then we never icrease the accessCount.

Is it unnecessary, or incorrect?

[Gabor Rapcsanyi]

(In reply to comment #2)
> Comment on attachment 249976 [details]
> proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249976&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Remove the ASSERT(m_pendingScript->accessCount() > 0) because if the MemoryCache is disabled
> > +        then we never icrease the accessCount.
> 
> Is it unnecessary, or incorrect?

Well, maybe incorrect is a better word for it. I'm not sure whether we should change the assert or just remove it.

[Chris Dumez]

Comment on attachment 249976 [details]
proposed patch

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

Couldn't we have a layout test for this by exposing MemoryCache::setDisabled() via Internals?

> Source/WebCore/xml/parser/XMLDocumentParser.cpp:-226
> -    ASSERT(m_pendingScript->accessCount() > 0);

Maybe ASSERT(m_pendingScript->accessCount() > 0 || MemoryCache::singleton().disabled()); then?

[Brady Eidson]

Comment on attachment 249976 [details]
proposed patch

This patch has been pending review since 2015 with no recent activity.
It seems unlikely that it would even still apply to trunk in its current form.

Clearing from the review queue.

Feel free to update and resubmit if the patch is still relevant.

[Ahmad Saleem]

>> [e-mail] - is this required?

https://github.com/WebKit/WebKit/blob/1ffe018f5d5cd17e88f1a34ee7a62d47290ed07d/Source/WebCore/xml/parser/XMLDocumentParser.cpp#L227

Seems like m_pendingScript is now changed to nullptr and it does not asset. Thanks!

[Alexey Proskuryakov]

There is no such assertion in XMLDocumentParser::notifyFinished any more.