Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[WebProfilerBundle] SetXDEBUG_IGNORE option for all XHR#52950

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:7.1fromadrolter:profiler_disable_xdebug
Mar 17, 2024

Conversation

@adrolter
Copy link
Contributor

@adrolteradrolter commentedDec 8, 2023
edited
Loading

QA
Branch?7.1
Bug fix?no
New feature?yes
Deprecations?no
IssuesN/A
LicenseMIT

When using an Xdebug browser extension with Symfony, toolbar requests are also processed by Xdebug because the browser extension has set anXDEBUG_* cookie on the page. This leads to superfluous breakpoint triggering when debugging at lower levels of the codebase, as well as a performance hit with no gain for every toolbar request, junk profiler/trace output blobs, etc.

The only sane way to prevent this behavior seems to be for the cookie to never be sent, as Xdebug has no mechanism for ignoring requests to certain URIs, and detecting this condition at the webserver and rewriting the request before passing it off to PHP is also non-trivial.

This PR detects theXDEBUG_* cookie and removes it before conducting XHR, then re-sets it after the fact.

Thanks!

@carsonbotcarsonbot added this to the7.1 milestoneDec 8, 2023
@adrolteradrolterforce-pushed theprofiler_disable_xdebug branch 2 times, most recently from8e0a86b to76241d9CompareDecember 8, 2023 18:31
@OskarStarkOskarStark changed the title[WebProfilerBundle] Remove XDEBUG_SESSION cookie while performing XHRs[WebProfilerBundle] RemoveXDEBUG_SESSION cookie while performing XHRsDec 9, 2023
xhr.open(options.method||'GET', url,true);
xhr.setRequestHeader('X-Requested-With','XMLHttpRequest');
xhr.onreadystatechange=function(state) {
if (xdebugCookieValue!==null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I don't fully understand why here we need this code to set the cookie 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

looks like the results are achieved by removing the cookie during the request. Therefor the cookie wil by set again after te request finishes.

adrolter and asrorbekh reacted with heart emoji
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You are right. This sounds obvious now 😊 Thanks for helping me understand this.

Copy link
ContributorAuthor

@adrolteradrolterDec 9, 2023
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, that's correct. Thank you, Duncan. The only pitfall I see with my approach is that it is possible that the request is interrupted and never finishes for some reason (which would leave the cookie unset), but this seems fairly unlikely. And even if it did happen, the worst outcome is that the Xdebug extension becomes disabled and has to manually be enabled again, so it's fairly benign.

ETA: I could add a "beforeunload" callback to handle this case, if deemed beneficial?

@adrolteradrolter marked this pull request as draftDecember 9, 2023 17:23
@adrolter
Copy link
ContributorAuthor

FYI I decided to implement the "beforeunload" handler and rework this a bit, so I've converted to it to a draft for the moment. I'll submit a revision and switch it back shortly. Thanks!

@adrolteradrolter changed the title[WebProfilerBundle] RemoveXDEBUG_SESSION cookie while performing XHRs[WebProfilerBundle] Temporarily removeXDEBUG_* cookie while performing XHRDec 9, 2023
@adrolteradrolter marked this pull request as ready for reviewDecember 9, 2023 18:49
@adrolter
Copy link
ContributorAuthor

Interrupted XHRs due to page reloads/redirects/etc. are now handled by abeforeunload listener, and trace/profile modes are now also supported (as well as any other future mode, as the patch now uses a regex to match). I also refactored the code to be localized to one function so that it's easier to follow.

@adrolteradrolter marked this pull request as draftDecember 9, 2023 22:13
@adrolteradrolter marked this pull request as ready for reviewDecember 9, 2023 23:20
@adrolter
Copy link
ContributorAuthor

adrolter commentedDec 9, 2023
edited
Loading

After more testing, I discovered that reinstating the cookie after the response was received (in areadystatechange callback) doesn't work, as taking longer than ~150ms to put the cookie back confuses the browser extensions. I reworked it again to reinstate the cookie after the request is sent, but this was also not so straightforward, because even though thedocumentation says that the request is sent by the timeXMLHttpRequest.send() returns, it's a filthy lie. 😭

What does seem to work very reliably, at least in Chrome, is reinstating the cookie on the next iteration of the browser's event loop.setTimeout(..., 0) was reliable over hundreds of iterations of testing the functionality in Chrome. But alas, Firefox was a slightly different story. ~5% of the time, Firefox hasn't sent the request for an extra iteration or two of the event loop. Setting the timeout delay to a reasonable value of 50ms seems to satisfy all parties on both Firefox and Chrome, but Safari remains untested.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Would it be possible to force the cookie header just for the XHR instead of changing the state of the browser?

@adrolter
Copy link
ContributorAuthor

@nicolas-grekas I wish, but no, XHR in modern browsers does not support manually specifying theCookie header for security reasons.

@adrolter
Copy link
ContributorAuthor

adrolter commentedDec 28, 2023
edited
Loading

As I see it, the only viable solutions to this issue are:

  1. What I've implemented here, which admittedly is very much a hack and last resort. From my experience using it heavily for the last few weeks it is a very reliable, well-functioning hack (within the specific environments I've tested it in, of course)...but it is a hack, nonetheless.

  2. Requiring users to somehow configure their webserver to rewrite requests to the/_wdt/ and/_profiler/ paths, removing theXDEBUG_* cookie in the process. Is this even possible with most common (Apache, Nginx, Caddy, ?) webservers? I guess if your profiler paths don't require authentication then you could more-or-less just drop theCookie header altogether...

I wish there was a more elegant avenue because I quite dislike both of the given options for different reasons, but I haven't found one yet.

@adrolter
Copy link
ContributorAuthor

Actually, what if instead of managing the Xdebug cookie via some extra browser extension, the Symfony toolbar could manage it instead through a new UI element with similar functionality to the browser extensions? This is probably still prone to side-effects when there are concurrent requests in different tabs or caused by XHR in the user's app, for example, but maybe it's a step in the right direction?

@nicolas-grekas
Copy link
Member

Another idea: since xdebug is sensitive to$_GET also, what would happen if we set those settings in the query string? Can this make xdebug ignore the cookie? Can you please check? 🙏

@adrolter
Copy link
ContributorAuthor

adrolter commentedDec 29, 2023
edited
Loading

Impossible, unfortunately...at least AFAICT. All of the GET parameters that Xdebug respects (XDEBUG_TRIGGER,XDEBUG_PROFILE,XDEBUG_TRACE,XDEBUG_SESSION, andXDEBUG_CONFIG) enable its functionality – none of them can disable it, because they are all "triggers" which selectively enable Xdebug in one or more modes for a single request (whenxdebug.start_with_request = trigger).

The only way to prevent triggering Xdebug when making an HTTP request andxdebug.start_with_request istrigger, is to never send one of these triggers via GET data, POST data, or the Cookie header to begin with.

TheXDEBUG_MODE environment variable can be set tooff to disable Xdebug...I exploit this in my dev environment to prevent the container from being rebuilt through Xdebug and taking an eternity to do so (using a lean console script that just initializes the kernel and exits,exec-d from insideKernel::initializeContainer() which then immediately returns a recursive call to itself), but I can't think of a way to make use of that mechanism (theXDEBUG_MODE environment variable) from the context of an HTTP client.

@derickr
Copy link

derickr commentedDec 31, 2023
edited
Loading

| Symfony toolbar could manage it instead through a new UI element with similar functionality to the browser extensions?

Please don't do that. That makes the support on my side even harder, as now there isanother way to trigger Xdebug.

All of the GET parameters that Xdebug respects (XDEBUG_TRIGGER, XDEBUG_PROFILE, XDEBUG_TRACE, XDEBUG_SESSION, and XDEBUG_CONFIG) enable its functionality

Does that also include passingXDEBUG_SESSION_STOP as GET parameter? I can't check right now, but I think that ought to work.

@adrolter
Copy link
ContributorAuthor

@derickr If that works, I won't complain, but it is undocumented. I'll report back with my findings – thanks!

That makes the support on my side even harder, as now there is another way to trigger Xdebug.

I hadn't considered this aspect. I assumed that support for the feature would fall on the Symfony project, but that was probably naive.

@adrolter
Copy link
ContributorAuthor

adrolter commentedDec 31, 2023
edited
Loading

XDEBUG_SESSION_STOP does not seem to have any effect. I do think I vaguely remember its existence years ago, but I'm still ending up on a breakpoint even with it set when I try it now (v3.2.2).

image

@nicolas-grekas
Copy link
Member

What aboutXDEBUG_SESSION_STOP_NO_EXEC?

@derickr
Copy link

No,XDEBUG_STOP_SESSION_NO_EXEC will halt execution of the script. This is something I am likely to remove. I'll investigate theXDEBUG_STOP_SESSION option in a bit. My todo list after the holidays is .... long, so it might take a while.

xabbuh and adrolter reacted with heart emoji

@derickr
Copy link

I have had a look now, and all thatXDEBUG_SESSION_STOP does is to unset theXDEBUG_SESSION cookie. It does not prevent the debugger from activating.

I see two options:

  • ChangeXDEBUG_SESSION_STOP to also inhibit the debugger from triggering
  • Add a newXDEBUG_IGNOREGET/POST/COOKIE option that ignores just the current request, but does not mess with theXDEBUG_SESSION system

I think I slightly prefer the latter, as I have the intention of phasing out the whole setting cookiesby Xdebug. Most people either use an IDE to trigger a request, or a browser extension which always sends the cookie, no matter what Xdebug does itself with the cookies. 🍪

adrolter reacted with thumbs up emoji

@adrolter
Copy link
ContributorAuthor

AnXDEBUG_IGNORE param would be awesome! Thank you very much for offering to help grease the wheels on this.

@derickr
Copy link

https://bugs.xdebug.org/view.php?id=2239

javiereguiluz and adrolter reacted with heart emoji

@nicolas-grekas
Copy link
Member

Thanks@derickr
Let's update this PR by anticipating the new option?

@derickr
Copy link

derickr commentedJan 23, 2024 via email

Just finishing it by writing the tests.

@derickr
Copy link

I have merged this into Xdebug's master now, in case you want to try it (xdebug/xdebug@5630bdb)

xabbuh and adrolter reacted with heart emoji

@nicolas-grekas
Copy link
Member

@adrolter let's resume this PR? 🙏

@adrolteradrolter changed the title[WebProfilerBundle] Temporarily removeXDEBUG_* cookie while performing XHR[WebProfilerBundle] SetXDEBUG_IGNORE option for all XHRFeb 10, 2024
@adrolter
Copy link
ContributorAuthor

adrolter commentedFeb 10, 2024
edited
Loading

Reworked to use thew newXDEBUG_IGNORE option.

@derickr Thank you very much for paving the way for this feature; it's very much appreciated!One thing I did notice in my testing is that – as opposed to my previous implementation where I prevented the cookie from being sent – this still seems to incur all the same CPU/time overhead of a normal Xdebug enabled request, but just doesn't stop at breakpoints. Is this expected, and related to what you were alluding to by saying that it would not mess with theXDEBUG_SESSION system? I just want to make sure I understand correctly how it is intended to work. Thanks again!Edit: Nevermind, I don't know what I was talking about; it behaves the same.

@fabpotfabpotforce-pushed theprofiler_disable_xdebug branch from745f20e toae07ed0CompareMarch 17, 2024 07:54
@fabpot
Copy link
Member

Thank you@adrolter.

@fabpotfabpot merged commit31b6a56 intosymfony:7.1Mar 17, 2024
@fabpotfabpot mentioned this pull requestMay 2, 2024
@adrolteradrolter deleted the profiler_disable_xdebug branchDecember 4, 2024 19:17
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@fabpotfabpotfabpot approved these changes

@javiereguiluzjaviereguiluzjaviereguiluz approved these changes

+1 more reviewer

@dsdeboerdsdeboerdsdeboer left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

7.1

Development

Successfully merging this pull request may close these issues.

7 participants

@adrolter@nicolas-grekas@derickr@fabpot@javiereguiluz@dsdeboer@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp