Bug 224678 – Web Inspector: add assertion for `WI.View` re-entrancy

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

.

[Devin Rousso]

Created attachment 426244 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/77083129>

[Blaze Burg]

Comment on attachment 426244 [details]
Patch

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

The usage of console.assert to have engineering-only reentrancy checks is clever. However, it's really hard to read. Could you instead use a helper function /lambda where this particular black magic lives?

It would be even clearer to have a ReentrancyChecker class with enter() and exit().

> Source/WebInspectorUI/ChangeLog:7
> +

Please explain why it's needed.

[Devin Rousso]

Created attachment 427630 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 427630 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/View.js:297
> +        console.assert(WI.setReentrantCheck(this, "layout"), "ERROR: calling `layout` while already in it", this);

If an exception happens in layout, we're probably going to hit this a bunch eh? (We shouldn't have exceptions in layout, I'm just imagining how this can happen.

[Joseph Pecoraro]

Comment on attachment 427630 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1789
> +WI.setReentrantCheck = function(object, key) {

Is this style normally to put opening brace on the following line? I'm fine either way.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1797
> +    --object[key];

Might read better as the `(object[key] || 0) - 1` in case the key doesn't exist:

>>> --({}['x'])
NaN

[Joseph Pecoraro]

Err, should we also remove this FIXME now? Or is there more to do?

    // FIXME: <https://webkit.org/b/224650> (Web Inspector: audit for re-entrancy issues with `initialLayout` and `layout`)

[Devin Rousso]

Comment on attachment 427630 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1789
>> +WI.setReentrantCheck = function(object, key) {
> 
> Is this style normally to put opening brace on the following line? I'm fine either way.

oh oops heh old habits 😅

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1797
>> +    --object[key];
> 
> Might read better as the `(object[key] || 0) - 1` in case the key doesn't exist:

I was assuming that it would never get called without `WI.clearReentrantCheck`, but I think you make a good point.  I'll change.

>> Source/WebInspectorUI/UserInterface/Views/View.js:297
>> +        console.assert(WI.setReentrantCheck(this, "layout"), "ERROR: calling `layout` while already in it", this);
> 
> If an exception happens in layout, we're probably going to hit this a bunch eh? (We shouldn't have exceptions in layout, I'm just imagining how this can happen.

Yeah it's possible for this to spiral out of control, but like you said if we have an exception I think there's a bigger issue 😅

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #7)
> Err, should we also remove this FIXME now? Or is there more to do?
> 
>     // FIXME: <https://webkit.org/b/224650> (Web Inspector: audit for re-entrancy issues with `initialLayout` and `layout`)

That bug is more about looking through the code and making sure other cases aren't present.  This bug is about trying to make that easier (or at least more noticeable).

[Devin Rousso]

Created attachment 427683 [details]
Patch

[EWS]

Committed r276975 (237304@main): <https://commits.webkit.org/237304@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427683 [details].