.
Created attachment 426244 [details] Patch
<rdar://problem/77083129>
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.
Created attachment 427630 [details] Patch
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.
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
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`)
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 😅
(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).
Created attachment 427683 [details] Patch
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].