Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Web Inspector: Debugger: further harden async stack trace code after 278872@main #28754

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dcrousso
Copy link
Member

@dcrousso dcrousso commented May 18, 2024

0d22aef

Web Inspector: Debugger: further harden async stack trace code after 278872@main
https://bugs.webkit.org/show_bug.cgi?id=274349

Reviewed by NOBODY (OOPS!).

It's probably true that 278872@main fixed the surface issue, but it might not have addressed the underlying problem. <#28619 (comment)>

This change represents a few speculative fixes that should hopefully further prevent any sort of issue like that.
- Use `InspectorDebuggeragent::currentParentStackTrace` wherever possible to avoid duplicating that code.
- Use `HashMap::get` instead of `HashMap::find` to also check that the returned pointer is valid.
- Only remove from `m_pendingAsyncCalls` if not `AsyncStackTrace::isPending` *and* if it's not also used elsewhere in `m_currentAsyncCallIdentifierStack`.
- Still call `AsyncStackTrace::didDispatchAsyncCall` even if there are no current async calls (i.e. just in case `InspectorDebuggerAgent::willDispatchAsyncCall` is somehow not called).

* Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:
(Inspector::InspectorDebuggerAgent::didScheduleAsyncCall):
(Inspector::InspectorDebuggerAgent::didCancelAsyncCall):
(Inspector::InspectorDebuggerAgent::willDispatchAsyncCall):
(Inspector::InspectorDebuggerAgent::didDispatchAsyncCall):
(Inspector::InspectorDebuggerAgent::didPause):

0d22aef

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2 βœ… πŸ§ͺ wincairo-tests
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-wpe
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ›  wpe-cairo
βœ… πŸ›  πŸ§ͺ jsc βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  gtk
βœ… πŸ›  πŸ§ͺ jsc-arm64 βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress βœ… πŸ§ͺ api-gtk
βœ… πŸ›  watch βœ… πŸ›  jsc-armv7
βœ… πŸ›  watch-sim βœ… πŸ§ͺ jsc-armv7-tests

…278872@main

https://bugs.webkit.org/show_bug.cgi?id=274349

Reviewed by NOBODY (OOPS!).

It's probably true that 278872@main fixed the surface issue, but it might not have addressed the underlying problem. <WebKit#28619 (comment)>

This change represents a few speculative fixes that should hopefully further prevent any sort of issue like that.
- Use `InspectorDebuggeragent::currentParentStackTrace` wherever possible to avoid duplicating that code.
- Use `HashMap::get` instead of `HashMap::find` to also check that the returned pointer is valid.
- Only remove from `m_pendingAsyncCalls` if not `AsyncStackTrace::isPending` *and* if it's not also used elsewhere in `m_currentAsyncCallIdentifierStack`.
- Still call `AsyncStackTrace::didDispatchAsyncCall` even if there are no current async calls (i.e. just in case `InspectorDebuggerAgent::willDispatchAsyncCall` is somehow not called).

* Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:
(Inspector::InspectorDebuggerAgent::didScheduleAsyncCall):
(Inspector::InspectorDebuggerAgent::didCancelAsyncCall):
(Inspector::InspectorDebuggerAgent::willDispatchAsyncCall):
(Inspector::InspectorDebuggerAgent::didDispatchAsyncCall):
(Inspector::InspectorDebuggerAgent::didPause):
@dcrousso dcrousso self-assigned this May 18, 2024
@dcrousso dcrousso added the Web Inspector Bugs related to the WebKit Web Inspector. label May 18, 2024
@francescorn
Copy link
Contributor

francescorn commented May 28, 2024

We could also rename it here to be more descriptive:

{
    auto it = m_scripts.find(parseIntegerAllowingTrailingJunk<JSC::SourceID>(scriptId).value_or(0));
    if (it == m_scripts.end())
        return makeUnexpected("Missing script for given scriptId"_s);
    return ContentSearchUtilities::searchInTextByLines(it->value.source, query, caseSensitive.value_or(false), isRegex.value_or(false));
}
Protocol::ErrorStringOr<String> InspectorDebuggerAgent::getScriptSource(const Protocol::Debugger::ScriptId& scriptId)
{
    auto it = m_scripts.find(parseIntegerAllowingTrailingJunk<JSC::SourceID>(scriptId).value_or(0));
    if (it == m_scripts.end())
        return makeUnexpected("Missing script for given scriptId"_s);
    return it->value.source;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Web Inspector Bugs related to the WebKit Web Inspector.
Projects
None yet
3 participants