Bug 148309 - Add RELEASE_ASSERT to RenderView's renderer counter.
Summary: Add RELEASE_ASSERT to RenderView's renderer counter.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-21 09:48 PDT by zalan
Modified: 2015-08-21 20:46 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.96 KB, patch)
2015-08-21 09:50 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (2.98 KB, patch)
2015-08-21 10:14 PDT, zalan
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (376.71 KB, application/zip)
2015-08-21 10:31 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2015-08-21 09:48:14 PDT
To check if we ever go below 0.
Comment 1 zalan 2015-08-21 09:50:36 PDT
Created attachment 259621 [details]
Patch
Comment 2 Simon Fraser (smfr) 2015-08-21 09:58:55 PDT
Comment on attachment 259621 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:134
> +    if (!m_node.isDocumentNode())
> +        view().didDestroyRenderer();

Seems a shame to hurt runtime with a !m_node.isDocumentNode() check here (on every renderer!), for an assert which I don't think would catch any bug that we know about?
Comment 3 zalan 2015-08-21 10:01:28 PDT
Comment on attachment 259621 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:135
>      ASSERT(!hasRareData());

This is about balancing the didCreate/didDestroy calls and probably should be fixed regardless of whether we end up asserting on m_rendererCount.
Comment 4 zalan 2015-08-21 10:03:35 PDT
(In reply to comment #3)
> Comment on attachment 259621 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259621&action=review
> 
> > Source/WebCore/rendering/RenderObject.cpp:135
> >      ASSERT(!hasRareData());
> 
> This is about balancing the didCreate/didDestroy calls and probably should
> be fixed regardless of whether we end up asserting on m_rendererCount.
Optionally we could remove these checks from both the c'tor and the d'tor.
Comment 5 zalan 2015-08-21 10:14:52 PDT
Created attachment 259624 [details]
Patch
Comment 6 Build Bot 2015-08-21 10:31:13 PDT
Comment on attachment 259624 [details]
Patch

Attachment 259624 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/86335

Number of test failures exceeded the failure limit.
Comment 7 Build Bot 2015-08-21 10:31:16 PDT
Created attachment 259628 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 8 zalan 2015-08-21 20:46:38 PDT
(In reply to comment #2)
> Comment on attachment 259621 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259621&action=review
> 
> > Source/WebCore/rendering/RenderObject.cpp:134
> > +    if (!m_node.isDocumentNode())
> > +        view().didDestroyRenderer();
> 
> Seems a shame to hurt runtime with a !m_node.isDocumentNode() check here (on
> every renderer!), for an assert which I don't think would catch any bug that
> we know about?
Actually it already caught this one bug 148352