Bug 137131 – Web Inspector: tests under LayoutTests/inspector/debugger are flaky

• Component: Web Inspector
• Status: RESOLVED FIXED

[Brian Burg]

This bug tracks diagnosing and unskipping the test flakiness.

[Radar WebKit Bug Importer]

<rdar://problem/18461335>

[Yury Semikhatsky]

Created attachment 379285 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 379285 [details]
Patch

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

> LayoutTests/ChangeLog:14
> +        * platform/gtk/inspector/debugger/breakpoints/resolved-dump-all-pause-locations-expected.txt: Added. I couldn't
> +        figure out why this essentially JSC-specific test would have different output on GTK but it does. Perhaps configuration
> +        in layout tests on GTK is slightly different.

Interesting! Do you have a diff between this and the baseline expected results? Was there a specific point in which the output changes?

[Yury Semikhatsky]

Created attachment 379294 [details]
testresolved-dump-all-pause-locations-pretty-diff

This is the diff. It's always the same. It's weird as it resolves breakpoint at line 98 first and then at 94.

[Yury Semikhatsky]

> Interesting! Do you have a diff between this and the baseline expected
> results? Was there a specific point in which the output changes?

The diff is always the same with 200+ iterations. I've spent some time trying to see where the difference comes from but gave up.

[Joseph Pecoraro]

(In reply to Yury Semikhatsky from comment #5)
> > Interesting! Do you have a diff between this and the baseline expected
> > results? Was there a specific point in which the output changes?
> 
> The diff is always the same with 200+ iterations. I've spent some time
> trying to see where the difference comes from but gave up.

Wow, very peculiar and somewhat concerning. It does look like it goes past a start position and comes back to it later.

Maybe there is an issue in the std::sort implementation?

    void DebuggerPausePositions::sort()
    {
        std::sort(m_positions.begin(), m_positions.end(), [] (const DebuggerPausePosition& a, const DebuggerPausePosition& b) {
            return a.position.offset < b.position.offset;
        });
    }

Maybe the sorted results are incorrect. We could try std::stable_sort just to see if results might match?

[Yury Semikhatsky]

(In reply to Joseph Pecoraro from comment #6)
> Wow, very peculiar and somewhat concerning. It does look like it goes past a
> start position and comes back to it later.
> 
> Maybe there is an issue in the std::sort implementation?
> 
>     void DebuggerPausePositions::sort()
>     {
>         std::sort(m_positions.begin(), m_positions.end(), [] (const
> DebuggerPausePosition& a, const DebuggerPausePosition& b) {
>             return a.position.offset < b.position.offset;
>         });
>     }
> 
> Maybe the sorted results are incorrect. We could try std::stable_sort just
> to see if results might match?

It turns out there is a couple of DebuggerPausePositions in DebuggerParseData whith exactly same position (equal offsets) but of type Enter and Pause:

(89,0) 1158 [1158]{2}
(89,6) 1164 [1158]{2}
(90,0) 1167 [1167]{2}
(90,7) 1174 [1167]{2}
(91,0) 1177 [1177]{0} <----
(91,0) 1177 [1177]{2} <----
(91,9) 1186 [1177]{2}
(91,11) 1188 [1177]{1}
(92,0) 1191 [1191]{0} <----
(92,0) 1191 [1191]{2} <----
(93,4) 1204 [1200]{2}
(94,0) 1206 [1206]{1}
(95,0) 1209 [1209]{0} <----
(95,0) 1209 [1209]{2} <----
(96,4) 1221 [1217]{2}
(97,0) 1228 [1228]{1}
(99,0) 1232 [1232]{2}

My hunch is that the binary search in breakpointLocationForLineColumn gets confused by that.

Is having multiple DebuggerPausePositions at the same offset legit? If so I can fix the resolution method to handle the case.

[Yury Semikhatsky]

I'm trying to make sense of the current expectations:

INSERTING AT: 89:8
PAUSES AT: 94:0
     86    
     87    x => x;
     88    () => 1;
 ->  89    (x) => x#;
     90    (x) => { x };
     91    (x) => {
     92        x
     93    };
 =>  94    |() => {
     95        var x;
     96    };
     97    
     98    var fObj = {
     99        f1: function() {

I'd expect (89:8) to resolve to the assignment at like 98 (in accord with the description of https://trac.webkit.org/changeset/206653) or to (90:0) which is is first immediately following break location. Joe, can you clarify what is actually expected to happen in this scenario?

[Joseph Pecoraro]

> Is having multiple DebuggerPausePositions at the same offset legit? If so I
> can fix the resolution method to handle the case.

I believe so, yes. They should have different types, but can be at the same offset.

> I'd expect (89:8) to resolve to the assignment at like 98 (in accord with
> the description of https://trac.webkit.org/changeset/206653) or to (90:0)
> which is is first immediately following break location. Joe, can you clarify
> what is actually expected to happen in this scenario?

I agree with what you're saying. Each of these is a statement, so it should resolve before the next statement. I'd expect (90:0).

[Yury Semikhatsky]

Created attachment 379462 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Blaze Burg]

Comment on attachment 379462 [details]
Patch

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

r=me, however I would also like Joe to also take a look.

> Source/JavaScriptCore/ChangeLog:19
> +        legtmost one first.

nit: leftmost

> Source/WebCore/ChangeLog:28
> +        in one batched but for now I'd like to keep it one per iteration.

nit: in one batch

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:51
> +        // We are guarenteed to have a Leave for an Entry so we don't need to bounds check.

nit: guaranteed

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:83
> +    for (++it; it != m_positions.end(); ++it) {

It's kind of unusual to put the ++ operator in the first clause, but I guess it's okay?

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:102
> +        if (m_hasTaskInQueue)

It would be more consistent to name this m_hasScheduledTask or similar. Whether RunLoop uses a queue or not shouldn't leak out to its clients, as it's not accessible.

[Devin Rousso]

Comment on attachment 379462 [details]
Patch

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

> Source/JavaScriptCore/debugger/Debugger.cpp:-364
> -    unsigned column = breakpoint.column ? breakpoint.column : Breakpoint::unspecifiedColumn;

There are a few other places that use `Breakpoint::unspecifiedColumn`.  Should we update those as well?
 - `Debugger::toggleBreakpoint`
 - `CodeBlock::hasOpDebugForLineAndColumn`
We could also then remove `Breakpoint::unspecifiedColumn` altogether =D

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:45
>  

Style: you could remove this newline.

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:54
> +                return Optional<JSTextPosition>(it->position);

I think this would be cleaner to read as:
```
    while (it->type == DebuggerPausePositionType::Enter)
        ++it;
    return it->position;
```

>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:83
>> +    for (++it; it != m_positions.end(); ++it) {
> 
> It's kind of unusual to put the ++ operator in the first clause, but I guess it's okay?

I agree.  I think I'd rather have the first `++it` be outside the `for`, and just have an empty first clause.

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:84
> +        DebuggerPausePosition& slidePosition = *it;

NIT: you could use `auto&`.

[Yury Semikhatsky]

Comment on attachment 379462 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:19
>> +        legtmost one first.
> 
> nit: leftmost

Done.

>> Source/WebCore/ChangeLog:28
>> +        in one batched but for now I'd like to keep it one per iteration.
> 
> nit: in one batch

Done.

>> Source/JavaScriptCore/debugger/Debugger.cpp:-364
>> -    unsigned column = breakpoint.column ? breakpoint.column : Breakpoint::unspecifiedColumn;
> 
> There are a few other places that use `Breakpoint::unspecifiedColumn`.  Should we update those as well?
>  - `Debugger::toggleBreakpoint`
>  - `CodeBlock::hasOpDebugForLineAndColumn`
> We could also then remove `Breakpoint::unspecifiedColumn` altogether =D

Done. The reason I touched this code in the first place was somewhat obscure conversion UINT_MAX to -1 while casting column from unsigned to int before passing it to breakpointLocationForLineColumn. I've updated this code to not rely on that and use int internally, while switching between signed/unsigned only in Debugger::resolveBreakpoint. It'd be even better to address https://webkit.org/b/162771 but it's outside the scope of this CL.

>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:45
>>  
> 
> Style: you could remove this newline.

Done.

>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:51
>> +        // We are guarenteed to have a Leave for an Entry so we don't need to bounds check.
> 
> nit: guaranteed

Done.

>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:54
>> +                return Optional<JSTextPosition>(it->position);
> 
> I think this would be cleaner to read as:
> ```
>     while (it->type == DebuggerPausePositionType::Enter)
>         ++it;
>     return it->position;
> ```

Done.

>>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:83
>>> +    for (++it; it != m_positions.end(); ++it) {
>> 
>> It's kind of unusual to put the ++ operator in the first clause, but I guess it's okay?
> 
> I agree.  I think I'd rather have the first `++it` be outside the `for`, and just have an empty first clause.

Done. Moved this increment above.

>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:84
>> +        DebuggerPausePosition& slidePosition = *it;
> 
> NIT: you could use `auto&`.

Done.

>> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:102
>> +        if (m_hasTaskInQueue)
> 
> It would be more consistent to name this m_hasScheduledTask or similar. Whether RunLoop uses a queue or not shouldn't leak out to its clients, as it's not accessible.

Agreed. Done.

[Yury Semikhatsky]

Created attachment 379517 [details]
Patch

[Devin Rousso]

Comment on attachment 379517 [details]
Patch

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

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1877
> +            if (line == opDebugLine && (!column.hasValue() || column == opDebugColumn))

You can drop the `hasValue`, as the `operator bool()` has the same effect.

> Source/JavaScriptCore/debugger/Breakpoint.h:-60
> -    static constexpr unsigned unspecifiedColumn = UINT_MAX;

Should we also make the `column` member variable `Optional<unsigned>` then?  That's much clearer IMO than having `0` be the "not set" value.

> Source/JavaScriptCore/debugger/Debugger.cpp:285
> +    if (column.hasValue()) {

Ditto

> Source/JavaScriptCore/debugger/Debugger.cpp:375
> +    Optional<JSTextPosition> resolvedPosition = parseData.pausePositions.breakpointLocationForLineColumn(line, column);

Interesting.  I'd expect these parameters to be `unsigned` and `Optional<unsigned>` too, but I suppose this is fine.

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:53
> +        return Optional<JSTextPosition>(it->position);

NIT: you shouldn't need the `Optional<JSTextPosition>`.

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:72
> +    DebuggerPausePosition& firstSlidePosition = *it++;

Doing this here means that we do extra work if the `firstSlidePosition` is valid to return.  We should keep the `++it` on it's own line.

Additionally, this is a pretty "dense" line of code, and future changes may easily confuse the fact that this _must_ be post-increment in order to function properly.  We can avoid that by making them separate.

[Yury Semikhatsky]

Comment on attachment 379517 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1877
>> +            if (line == opDebugLine && (!column.hasValue() || column == opDebugColumn))
> 
> You can drop the `hasValue`, as the `operator bool()` has the same effect.

Done. The reason I used hasValue() as it's more obvious what is the expected behavior when *column == 0 and there is no implicit unsigned => bool conversion will be involved. But since Optional doesn't have operator T() it should be ok to use !column too.

>> Source/JavaScriptCore/debugger/Breakpoint.h:-60
>> -    static constexpr unsigned unspecifiedColumn = UINT_MAX;
> 
> Should we also make the `column` member variable `Optional<unsigned>` then?  That's much clearer IMO than having `0` be the "not set" value.

That may make sense, I haven't looked at all usages of the struct yet to tell. Let's contain the scope of this change to what is relevant to test flakiness. The refactoring you mention can be done in its own CL.

>> Source/JavaScriptCore/debugger/Debugger.cpp:285
>> +    if (column.hasValue()) {
> 
> Ditto

Done.

>> Source/JavaScriptCore/debugger/Debugger.cpp:375
>> +    Optional<JSTextPosition> resolvedPosition = parseData.pausePositions.breakpointLocationForLineColumn(line, column);
> 
> Interesting.  I'd expect these parameters to be `unsigned` and `Optional<unsigned>` too, but I suppose this is fine.

At some point we need to define what position line, <no column> refers to and I'd rather keep that place as close to the protocol handler (i.e. debugger agent) as possible and ideally do it a single place. It'd be better if pause positions map was not in the business of dealing with missing column and carried single function which is finding closest pause location for given arguments. Do you see any advantage of making it Optional argument of breakpointLocationForLineColumn?

>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:53
>> +        return Optional<JSTextPosition>(it->position);
> 
> NIT: you shouldn't need the `Optional<JSTextPosition>`.

Done. That's the way it was written before.

>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:72
>> +    DebuggerPausePosition& firstSlidePosition = *it++;
> 
> Doing this here means that we do extra work if the `firstSlidePosition` is valid to return.  We should keep the `++it` on it's own line.
> 
> Additionally, this is a pretty "dense" line of code, and future changes may easily confuse the fact that this _must_ be post-increment in order to function properly.  We can avoid that by making them separate.

Done. I don't think performance is a concern here, also good compiler might be able to optimize that properly.

I've changed it to the following as you guys suggested:

    ++it;
    for (; it != m_positions.end(); ++it) {

[Yury Semikhatsky]

Created attachment 379558 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 379558 [details]
Patch

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

Awesome!

> Source/JavaScriptCore/ChangeLog:9
> +        Changed breakpoint resolution logic to make it cosistent across platforms and

Typo: "cosistent" => "consistent"

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:113
> +    void dispatchOneMessage()
>      {
>          ASSERT(m_inspectedPageController);

The WebKit1 bot seems to crash:

    Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
    0   com.apple.JavaScriptCore      	0x0000000103ac5ce0 WTFCrash + 16
    1   com.apple.WebCore             	0x0000000111b77c2b WTFCrashWithInfo(int, char const*, char const*, int) + 27
    2   com.apple.WebCore             	0x00000001146d5c79 WebCore::InspectorBackendDispatchTask::dispatchOneMessage() + 105
    3   com.apple.WebCore             	0x00000001146d5bfc WebCore::InspectorBackendDispatchTask::scheduleOneShot()::'lambda'()::operator()() const + 28
    4   com.apple.WebCore             	0x00000001146d59a9 WTF::Detail::CallableWrapper<WebCore::InspectorBackendDispatchTask::scheduleOneShot()::'lambda'(), void>::call() + 25

Perhaps the assert is catching a real issue. Maybe we need to upgrade this assertion to an early return.

[Yury Semikhatsky]

Comment on attachment 379558 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        Changed breakpoint resolution logic to make it cosistent across platforms and
> 
> Typo: "cosistent" => "consistent"

Done.

>> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:113
>>          ASSERT(m_inspectedPageController);
> 
> The WebKit1 bot seems to crash:
> 
>     Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
>     0   com.apple.JavaScriptCore      	0x0000000103ac5ce0 WTFCrash + 16
>     1   com.apple.WebCore             	0x0000000111b77c2b WTFCrashWithInfo(int, char const*, char const*, int) + 27
>     2   com.apple.WebCore             	0x00000001146d5c79 WebCore::InspectorBackendDispatchTask::dispatchOneMessage() + 105
>     3   com.apple.WebCore             	0x00000001146d5bfc WebCore::InspectorBackendDispatchTask::scheduleOneShot()::'lambda'()::operator()() const + 28
>     4   com.apple.WebCore             	0x00000001146d59a9 WTF::Detail::CallableWrapper<WebCore::InspectorBackendDispatchTask::scheduleOneShot()::'lambda'(), void>::call() + 25
> 
> Perhaps the assert is catching a real issue. Maybe we need to upgrade this assertion to an early return.

The reason it's failing is the following:

The following code in inspector-test.js

TestPage.addResult = function(text)
{
    // For early errors triggered when loading the test page, write to stderr.
    if (!document.body) {
        this.debugLog(text);
        this.completeTest();
    }

will trigger

    // Close inspector asynchrously in case we want to test tear-down behavior.
    setTimeout(() => {
        testRunner.closeWebInspector();

At the same time runTestCasesAndFinish in the front-end will call FrontendTestHarness.completeTest() which evaluates the same TestPage.completeTest() in the inspected page. It creates a race between testRunner.closeWebInspector() and dispatch of the eval of TestPage.completeTest(). When inspector is closed before dispatching the latter command the assertion fails.

The dispatcher should not assume that the front-end is still alive at the moment it is trigger by RunLoop as it can be closed independently. I'll update the logic.

[Yury Semikhatsky]

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        Changed breakpoint resolution logic to make it cosistent across platforms and
> 
> Typo: "cosistent" => "consistent"

Done.

>> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:113
>>          ASSERT(m_inspectedPageController);
> 
> The WebKit1 bot seems to crash:
> 
>     Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
>     0   com.apple.JavaScriptCore      	0x0000000103ac5ce0 WTFCrash + 16
>     1   com.apple.WebCore             	0x0000000111b77c2b WTFCrashWithInfo(int, char const*, char const*, int) + 27
>     2   com.apple.WebCore             	0x00000001146d5c79 WebCore::InspectorBackendDispatchTask::dispatchOneMessage() + 105
>     3   com.apple.WebCore             	0x00000001146d5bfc WebCore::InspectorBackendDispatchTask::scheduleOneShot()::'lambda'()::operator()() const + 28
>     4   com.apple.WebCore             	0x00000001146d59a9 WTF::Detail::CallableWrapper<WebCore::InspectorBackendDispatchTask::scheduleOneShot()::'lambda'(), void>::call() + 25
> 
> Perhaps the assert is catching a real issue. Maybe we need to upgrade this assertion to an early return.

The reason it's failing is the following:

The following code in inspector-test.js

TestPage.addResult = function(text)
{
    // For early errors triggered when loading the test page, write to stderr.
    if (!document.body) {
        this.debugLog(text);
        this.completeTest();
    }

will trigger

    // Close inspector asynchrously in case we want to test tear-down behavior.
    setTimeout(() => {
        testRunner.closeWebInspector();

At the same time runTestCasesAndFinish in the front-end will call FrontendTestHarness.completeTest() which evaluates the same TestPage.completeTest() in the inspected page. It creates a race between testRunner.closeWebInspector() and dispatch of the eval of TestPage.completeTest(). When inspector is closed before dispatching the latter command the assertion fails.

The dispatcher should not assume that the front-end is still alive at the moment it is trigger by RunLoop as it can be closed independently. I'll update the logic.

[Yury Semikhatsky]

Created attachment 379664 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 379664 [details]
Patch

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

r=me, thanks for iterating!

> Source/JavaScriptCore/ChangeLog:30
> +        (JSC::DebuggerPausePositions::sort): use type as secondary ordering component.
> +        * debugger/DebuggerParseData.h: Rearranged type constants so that Enter < Pause < Leave
> +        this change along with sorting by type should guarantee that in case of several pause

This might deserve a comment in code since it is not immediately apparent by looking at the Enum that the order is required by comparisons later.

[Yury Semikhatsky]

Comment on attachment 379664 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:30
>> +        this change along with sorting by type should guarantee that in case of several pause
> 
> This might deserve a comment in code since it is not immediately apparent by looking at the Enum that the order is required by comparisons later.

Done.

[Yury Semikhatsky]

Created attachment 379743 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 379743 [details]
Patch

r=me

[Devin Rousso]

Comment on attachment 379743 [details]
Patch

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

also r=me, with some minor remaining NIT/Style comments.

> Source/JavaScriptCore/debugger/Debugger.cpp:380
> +    int resolvedColumn = resolvedPosition->offset - resolvedPosition->lineStartOffset;

Can this use `resolvePosition->column()`?

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:36
> +    DebuggerPausePosition position;

Should we add a fourth "invalid" value to `DebuggerPausePositionType` so that we don't have a default/undefined value for `position.type`?

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:38
> +    position.position.line = line;
> +    position.position.offset = column;

NIT: you could use uniform initialization syntax:
```
    DebuggerPausePosition position = { DebuggerPausePositionType::Invalid, JSTextPosition(line, column, 0) };
```

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:39
> +    position.position.lineStartOffset = 0;

NIT: this defaults to `0`, so is it necessary to specify it again here?

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:72
> +    DebuggerPausePosition& firstSlidePosition = *it;

Style: `auto&`

[Yury Semikhatsky]

Comment on attachment 379743 [details]
Patch

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

>> Source/JavaScriptCore/debugger/Debugger.cpp:380
>> +    int resolvedColumn = resolvedPosition->offset - resolvedPosition->lineStartOffset;
> 
> Can this use `resolvePosition->column()`?

Done.

>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:36
>> +    DebuggerPausePosition position;
> 
> Should we add a fourth "invalid" value to `DebuggerPausePositionType` so that we don't have a default/undefined value for `position.type`?

Done.

>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:38
>> +    position.position.offset = column;
> 
> NIT: you could use uniform initialization syntax:
> ```
>     DebuggerPausePosition position = { DebuggerPausePositionType::Invalid, JSTextPosition(line, column, 0) };
> ```

Done.

>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:39
>> +    position.position.lineStartOffset = 0;
> 
> NIT: this defaults to `0`, so is it necessary to specify it again here?

It makes the intent more obvious and doesn't require looking at what default is (0, -1 or what not). Replaced this code with initializer list anyway.

>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:72
>> +    DebuggerPausePosition& firstSlidePosition = *it;
> 
> Style: `auto&`

Done. Is there a rule for using auto vs. explicit type in webkit, I don't see it in the main style guide?

[Yury Semikhatsky]

Created attachment 379874 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 379874 [details]
Patch

Clearing flags on attachment: 379874

Committed r250655: <https://trac.webkit.org/changeset/250655>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/250655/webkit

has caused two tests to become flakey failures:
inspector/layers/layers-for-node.html 
inspector/timeline/line-column.html

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Flayers%2Flayers-for-node.html%20inspector%2Ftimeline%2Fline-column.html

Diff:
https://build.webkit.org/results/Apple%20Mojave%20Release%20WK1%20(Tests)/r250672%20(8069)/inspector/layers/layers-for-node-diff.txt
https://build.webkit.org/results/Apple%20Mojave%20Release%20WK1%20(Tests)/r250672%20(8069)/inspector/timeline/line-column-diff.txt

I was able to reproduce this locally by running these tests on a build of 250654 which passed in 2000 iterations, and on 250655 which failed almost instantly. 

These are severely flakey on the Mac queues, can this be resolved today? Otherwise we will need to roll this back.

[Yury Semikhatsky]

(In reply to Truitt Savell from comment #32)
stantly. 
> 
> These are severely flakey on the Mac queues, can this be resolved today?
> Otherwise we will need to roll this back.
Can we mark the two tests as flaky for now, I'll have a look tomorrow?

[Yury Semikhatsky]

Reopening to attach new patch.

[Yury Semikhatsky]

Created attachment 380254 [details]
Patch

[Yury Semikhatsky]

Created attachment 380255 [details]
initial layer tree diff

Initial layer tree sometimes has too small size of one of the layers like in this example. It feels like the test should be waiting for some key events to be sure that the layer sizes already computed. I'd appreciate any help from people who understand how this layer tree inspection works.

[Devin Rousso]

Comment on attachment 380254 [details]
Patch

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

rs=me

> LayoutTests/ChangeLog:3
> +        Web Inspector: tests under LayoutTests/inspector/debugger are flaky

It's a bit odd to have the title of this bug be about debugger when both of the tests being modified in this patch aren't related to the debugger at all.  I'd either make a new bug, or include some explanation about how r250655 caused these tests to become "more" flakey.  My guess is the changes made to use the `RunLoop` instead of a `Timer` in `InspectorFrontendClientLocal`.  Some explanation of why this happened would be great :)

> LayoutTests/inspector/layers/layers-for-node.html:29
> +            callback: getDocument.bind(this, layersChanged)

This is really odd.  We usually try to avoid binding arguments like this.  Instead, please just create `let layersChanged = null;` outside this function and set it here, then use it inside `getDocument` (just like `documentNode` and `initialLayers`).

> LayoutTests/inspector/layers/layers-for-node.html:35
> +        layersChangedPromise.then(step({

`step` doesn't return a function, so I don't think this works as you expect.  You'll need to wrap it in a function.
```js
    layersChangedPromise.then(() => {
        step({
            ...
        });
    });
```

> LayoutTests/inspector/timeline/line-column.html:44
> +    const eventWhitelist = new Set(["RenderingFrame", "ConsoleProfile", "EventDispatch", "FunctionCall"]);

Interesting idea!  Other than "ScheduleStyleRecalculation", were there any other events that would sometimes get recorded? :P

[Yury Semikhatsky]

Comment on attachment 380254 [details]
Patch

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

Thanks for the review! I will upload updated patch to https://webkit.org/b/202649 to avoid confusion.

>> LayoutTests/ChangeLog:3
>> +        Web Inspector: tests under LayoutTests/inspector/debugger are flaky
> 
> It's a bit odd to have the title of this bug be about debugger when both of the tests being modified in this patch aren't related to the debugger at all.  I'd either make a new bug, or include some explanation about how r250655 caused these tests to become "more" flakey.  My guess is the changes made to use the `RunLoop` instead of a `Timer` in `InspectorFrontendClientLocal`.  Some explanation of why this happened would be great :)

The root cause definitely has to do with the switch from Timer queue to RunLoop queue. Apparently timer events were fired after a longer delay than RunLoop events.  I don't have enough knowledge about the rendering layers to say what the test might have relied upon. graouts@ who wrote added the test may know the answer. I've updated potentially racy places in the test and it passes now.

>> LayoutTests/inspector/layers/layers-for-node.html:29
>> +            callback: getDocument.bind(this, layersChanged)
> 
> This is really odd.  We usually try to avoid binding arguments like this.  Instead, please just create `let layersChanged = null;` outside this function and set it here, then use it inside `getDocument` (just like `documentNode` and `initialLayers`).

Changed. I'm not sure it's better to add a bunch of unrelated variables to the outer scope (each of which is used just to pass some piece of information between two functions) than passing the same data only to the function where they are used but since the rest of the code uses former approach I'll follow the same style. I'm curious to learn more about the reasons you prefer one style over the other.

>> LayoutTests/inspector/layers/layers-for-node.html:35
>> +        layersChangedPromise.then(step({
> 
> `step` doesn't return a function, so I don't think this works as you expect.  You'll need to wrap it in a function.
> ```js
>     layersChangedPromise.then(() => {
>         step({
>             ...
>         });
>     });
> ```

Nice catch, thanks! This actually eliminated remaining flakiness in the initial tree that I mentioned in the bug. I've managed to run the test with --iterations=10000 and 0 failures.

>> LayoutTests/inspector/timeline/line-column.html:44
>> +    const eventWhitelist = new Set(["RenderingFrame", "ConsoleProfile", "EventDispatch", "FunctionCall"]);
> 
> Interesting idea!  Other than "ScheduleStyleRecalculation", were there any other events that would sometimes get recorded? :P

This is the only one that I've seen while running the test with --iterations=1000.

[Yury Semikhatsky]

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

Thanks for the review! I will upload updated patch to https://webkit.org/b/202649 to avoid confusion.

>> LayoutTests/ChangeLog:3
>> +        Web Inspector: tests under LayoutTests/inspector/debugger are flaky
> 
> It's a bit odd to have the title of this bug be about debugger when both of the tests being modified in this patch aren't related to the debugger at all.  I'd either make a new bug, or include some explanation about how r250655 caused these tests to become "more" flakey.  My guess is the changes made to use the `RunLoop` instead of a `Timer` in `InspectorFrontendClientLocal`.  Some explanation of why this happened would be great :)

The root cause definitely has to do with the switch from Timer queue to RunLoop queue. Apparently timer events were fired after a longer delay than RunLoop events.  I don't have enough knowledge about the rendering layers to say what the test might have relied upon. graouts@ who wrote added the test may know the answer. I've updated potentially racy places in the test and it passes now.

>> LayoutTests/inspector/layers/layers-for-node.html:29
>> +            callback: getDocument.bind(this, layersChanged)
> 
> This is really odd.  We usually try to avoid binding arguments like this.  Instead, please just create `let layersChanged = null;` outside this function and set it here, then use it inside `getDocument` (just like `documentNode` and `initialLayers`).

Changed. I'm not sure it's better to add a bunch of unrelated variables to the outer scope (each of which is used just to pass some piece of information between two functions) than passing the same data only to the function where they are used but since the rest of the code uses former approach I'll follow the same style. I'm curious to learn more about the reasons you prefer one style over the other.

>> LayoutTests/inspector/layers/layers-for-node.html:35
>> +        layersChangedPromise.then(step({
> 
> `step` doesn't return a function, so I don't think this works as you expect.  You'll need to wrap it in a function.
> ```js
>     layersChangedPromise.then(() => {
>         step({
>             ...
>         });
>     });
> ```

Nice catch, thanks! This actually eliminated remaining flakiness in the initial tree that I mentioned in the bug. I've managed to run the test with --iterations=10000 and 0 failures.

>> LayoutTests/inspector/timeline/line-column.html:44
>> +    const eventWhitelist = new Set(["RenderingFrame", "ConsoleProfile", "EventDispatch", "FunctionCall"]);
> 
> Interesting idea!  Other than "ScheduleStyleRecalculation", were there any other events that would sometimes get recorded? :P

This is the only one that I've seen while running the test with --iterations=1000.

[Truitt Savell]

Is there any further movement on this?

[Yury Semikhatsky]

(In reply to Truitt Savell from comment #40)
> Is there any further movement on this?

There is a bunch of problems with individual debugger tests, each of them have its own bugs. E.g. see https://bugs.webkit.org/show_bug.cgi?id=128736 and patch with a fix for it. If there are new issues, it'd be better to file bugs with more specific problem descriptions for the failing tests as the root causes of their failures are likely different too.