Bug 97641 – Web Inspector: compare objects counted by the memory instrumentation with those allocated in the heap

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Yury Semikhatsky]

We need to be sure that the memory instrumentation only counts size of objects allocated in the heap. To test that we could request a set of all allocated objects from the memory allocator and check if the counted objects set is a subset of the set of all heap allocated objects.

[Yury Semikhatsky]

Created attachment 165743 [details]
Possible implementation draft

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 165743 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/wtf/MemoryInstrumentation.h', u..." exit_code: 1
Source/WebCore/inspector/MemoryInstrumentationImpl.cpp:80:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/inspector/InspectorMemoryAgent.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/src/WebDevToolsAgentImpl.h:109:  The parameter name "set" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/InspectorClientImpl.h:81:  The parameter name "set" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 165743 [details]
Possible implementation draft 

Attachment 165743 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14027415

[Build Bot]

Comment on attachment 165743 [details]
Possible implementation draft 

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

[WebKit Review Bot]

Comment on attachment 165743 [details]
Possible implementation draft 

Attachment 165743 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14018429

[Peter Beverloo (cr-android ews)]

Comment on attachment 165743 [details]
Possible implementation draft 

Attachment 165743 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14004033

[Yury Semikhatsky]

Created attachment 165753 [details]
Patch

[Pavel Feldman]

Comment on attachment 165753 [details]
Patch

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

> Source/WTF/wtf/MemoryInstrumentation.h:110
> +#if ENABLE(DEBUG_MEMORY_INSTRUMENTATION)

I would only guard the backing implementation for better readability.

> Source/WebCore/inspector/InspectorClient.h:75
> +#if ENABLE(DEBUG_MEMORY_INSTRUMENTATION)

I would even remove this guard.

> Source/WebCore/inspector/MemoryInstrumentationImpl.cpp:86
> +    if (!m_allocatedObjects.contains(object)) {

You could only consider non-empty arrays valid.

[Yury Semikhatsky]

Created attachment 165763 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #9)
> (From update of attachment 165753 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165753&action=review
> 
> > Source/WTF/wtf/MemoryInstrumentation.h:110
> > +#if ENABLE(DEBUG_MEMORY_INSTRUMENTATION)
> 
> I would only guard the backing implementation for better readability.
> 
Done.

> > Source/WebCore/inspector/InspectorClient.h:75
> > +#if ENABLE(DEBUG_MEMORY_INSTRUMENTATION)
> 
> I would even remove this guard.
> 
Done.

> > Source/WebCore/inspector/MemoryInstrumentationImpl.cpp:86
> > +    if (!m_allocatedObjects.contains(object)) {
> 
> You could only consider non-empty arrays valid.
Done.

[Pavel Feldman]

Comment on attachment 165763 [details]
Patch

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

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:492
> +            : m_totalObjectsCount(totalObjectsCount + 1)

I would multiply it by 2 and add a comment that the first pass was only estimating the number of objects for this allocation. If the *2 size is not enough, you could double it again and re-run as a fall back.

[Yury Semikhatsky]

(In reply to comment #12)
> (From update of attachment 165763 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165763&action=review
> 
> > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:492
> > +            : m_totalObjectsCount(totalObjectsCount + 1)
> 
> I would multiply it by 2 and add a comment that the first pass was only estimating the number of objects for this allocation. If the *2 size is not enough, you could double it again and re-run as a fall back.

Done.

[Yury Semikhatsky]

Created attachment 165765 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 165765 [details]
Patch

Attachment 165765 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14035385

[Pavel Feldman]

Comment on attachment 165765 [details]
Patch

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

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:506
> +                printf("Adding more than %lu pointers.", m_maxObjectsCount);

Please remove this.

[Yury Semikhatsky]

Committed r129623: <http://trac.webkit.org/changeset/129623>