Bug 143334 - Remove unnecessary ASSERT from XMLDocumentParser::notifyFinished()
Summary: Remove unnecessary ASSERT from XMLDocumentParser::notifyFinished()
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-02 06:42 PDT by Gabor Rapcsanyi
Modified: 2017-04-24 19:07 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (1.40 KB, patch)
2015-04-02 06:50 PDT, Gabor Rapcsanyi
beidson: review-
beidson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabor Rapcsanyi 2015-04-02 06:42:31 PDT
Remove the ASSERT(m_pendingScript->accessCount() > 0) because if the MemoryCache is disabled then we never icrease the accessCount.
Comment 1 Gabor Rapcsanyi 2015-04-02 06:50:55 PDT
Created attachment 249976 [details]
proposed patch
Comment 2 Alexey Proskuryakov 2015-04-02 22:15:53 PDT
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?
Comment 3 Gabor Rapcsanyi 2015-04-03 03:46:21 PDT
(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.
Comment 4 Chris Dumez 2015-04-16 14:53:27 PDT
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?
Comment 5 Brady Eidson 2017-04-24 19:06:51 PDT
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.