Bug 112596 - Web Inspector: Auto expand retaining path until there are more than one retainer.
Summary: Web Inspector: Auto expand retaining path until there are more than one retai...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexei Filippov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-18 10:55 PDT by Alexei Filippov
Modified: 2013-03-28 02:48 PDT (History)
10 users (show)

See Also:


Attachments
Patch (2.42 KB, patch)
2013-03-18 10:59 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (12.84 KB, patch)
2013-03-20 07:31 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (15.77 KB, patch)
2013-03-21 02:18 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (998.57 KB, application/zip)
2013-03-21 03:02 PDT, WebKit Review Bot
no flags Details
Patch (15.90 KB, patch)
2013-03-21 08:29 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (16.56 KB, patch)
2013-03-27 08:03 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexei Filippov 2013-03-18 10:55:11 PDT
When retainers view gets opened automatically expand retaining path until reaching an object that has more than one retainer.
Comment 1 Alexei Filippov 2013-03-18 10:59:58 PDT
Created attachment 193606 [details]
Patch
Comment 2 Pavel Feldman 2013-03-19 09:38:10 PDT
Comment on attachment 193606 [details]
Patch

Please provide a test.
Comment 3 Alexei Filippov 2013-03-20 07:31:23 PDT
Created attachment 194050 [details]
Patch
Comment 4 Alexei Filippov 2013-03-20 07:59:04 PDT
(In reply to comment #2)
> (From update of attachment 193606 [details])
> Please provide a test.

Added a test.
Comment 5 Yury Semikhatsky 2013-03-20 08:05:59 PDT
(In reply to comment #0)
> When retainers view gets opened automatically expand retaining path until reaching an object that has more than one retainer.

What about expanding shortest path to the Window?
Comment 6 Yury Semikhatsky 2013-03-20 08:10:37 PDT
Comment on attachment 194050 [details]
Patch

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

> Source/WebCore/inspector/front-end/HeapSnapshotDataGrids.js:508
> +            if (this.children.length === 1) {

Can it happen that we didn't receive response from the worker and don't know number of children here?
Comment 7 Alexei Filippov 2013-03-21 02:18:07 PDT
Created attachment 194206 [details]
Patch
Comment 8 Alexei Filippov 2013-03-21 02:22:42 PDT
Comment on attachment 194050 [details]
Patch

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

>> Source/WebCore/inspector/front-end/HeapSnapshotDataGrids.js:508
>> +            if (this.children.length === 1) {
> 
> Can it happen that we didn't receive response from the worker and don't know number of children here?

AFAICT populateComplete event arrives on response from the worker. If for some reason there's no response it just stops expanding the chain.
Comment 9 Alexei Filippov 2013-03-21 02:23:56 PDT
(In reply to comment #5)
> (In reply to comment #0)
> > When retainers view gets opened automatically expand retaining path until reaching an object that has more than one retainer.
> 
> What about expanding shortest path to the Window?

Good idea.
However it sounds like a task for a separate CL.
Comment 10 WebKit Review Bot 2013-03-21 03:02:05 PDT
Comment on attachment 194206 [details]
Patch

Attachment 194206 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17112602

New failing tests:
inspector/profiler/heap-snapshot-summary-retainers.html
platform/chromium/fast/forms/calendar-picker/calendar-picker-appearance-month-popup.html
Comment 11 WebKit Review Bot 2013-03-21 03:02:09 PDT
Created attachment 194212 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 12 Alexei Filippov 2013-03-21 08:29:18 PDT
Created attachment 194265 [details]
Patch
Comment 13 Yury Semikhatsky 2013-03-27 02:11:10 PDT
Comment on attachment 194265 [details]
Patch

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

> LayoutTests/inspector/profiler/heap-snapshot-summary-retainers.html:106
> +                setTimeout(step5.bind(null, retainersRoot), 500);

This is bound to flake. Please replace the timeout with some deterministic condition.

> LayoutTests/inspector/profiler/heap-snapshot-summary-retainers.html:183
> +                setTimeout(step5.bind(null, retainersRoot), 500);

Ditto.
Comment 14 Alexei Filippov 2013-03-27 02:23:24 PDT
Comment on attachment 194265 [details]
Patch

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

>> LayoutTests/inspector/profiler/heap-snapshot-summary-retainers.html:106
>> +                setTimeout(step5.bind(null, retainersRoot), 500);
> 
> This is bound to flake. Please replace the timeout with some deterministic condition.

ok

>> LayoutTests/inspector/profiler/heap-snapshot-summary-retainers.html:183
>> +                setTimeout(step5.bind(null, retainersRoot), 500);
> 
> Ditto.

This one is not a flake. It is a deterministic poll loop. It will wait until there's at least 20 items expanded. See lines 189-190.
Comment 15 Yury Semikhatsky 2013-03-27 02:28:04 PDT
Comment on attachment 194265 [details]
Patch

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

>>> LayoutTests/inspector/profiler/heap-snapshot-summary-retainers.html:183
>>> +                setTimeout(step5.bind(null, retainersRoot), 500);
>> 
>> Ditto.
> 
> This one is not a flake. It is a deterministic poll loop. It will wait until there's at least 20 items expanded. See lines 189-190.

You should be able to listen to some event instead of polling with 500ms interval.
Comment 16 Alexei Filippov 2013-03-27 08:03:52 PDT
Created attachment 195323 [details]
Patch
Comment 17 Alexei Filippov 2013-03-27 08:04:23 PDT
Comment on attachment 194265 [details]
Patch

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

>>>> LayoutTests/inspector/profiler/heap-snapshot-summary-retainers.html:183
>>>> +                setTimeout(step5.bind(null, retainersRoot), 500);
>>> 
>>> Ditto.
>> 
>> This one is not a flake. It is a deterministic poll loop. It will wait until there's at least 20 items expanded. See lines 189-190.
> 
> You should be able to listen to some event instead of polling with 500ms interval.

done
Comment 18 WebKit Review Bot 2013-03-28 02:48:16 PDT
Comment on attachment 195323 [details]
Patch

Clearing flags on attachment: 195323

Committed r147090: <http://trac.webkit.org/changeset/147090>
Comment 19 WebKit Review Bot 2013-03-28 02:48:20 PDT
All reviewed patches have been landed.  Closing bug.