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
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

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