I've installed xhprof from source following the instructions here, but xhprof isn't showing up when I run php -i. Once I figure that out I'll be able to actually test this out :D

Another question, is there a way to link this diff to its underlying task (https://secure.phabricator.com/T9690) ?

Oh, right. The frame issue is common in both XHProf and XHPAST, since they both support embedding in <iframe /> tags.

Normally, we emit an X-Frame-Options: Deny HTTP header on every response by default. This stops browers from embedding the page in an iframe, and prevents various attacks where you embed a page with a dangerous button on it (say, "Accept Revision") in a tiny iframe on an innocent looking page, then put something shiny looking over the <iframe />, then entice the user to click the shiny thing. When they do, you remove the shiny thing from the DOM real fast so the click hits the dangerous button. Since they're logged in and the page is iframed, the click works and takes the action without the user's knowledge or intent.

However, XHProf and XHPAST use iframes legitimately. XHProf uses them in the DarkConsole panel (the upper right section with profiling information is an <iframe />):

...while XHPAST uses them to render columns (the three columns here are <frame /> tags):

Both of these UIs are unusual, and one possible approach would be to remove the use of frames completely. They're used in XHPAST only because it as easier than writing flexible accordion boxes when I wrote the UI in 2011, and could easily be replaced with a table with no real loss of functionality. Likewise, there's no real reason the DarkConsole element needs to be a frame.

However, these are larger changes that would probably be better to pursue alongside adjacent work in the applications (XHProf is due for a more general update, in particular). For now, let's just make frames work with newPage() so we can clean up buildStandardPageResponse().

To do this:

• • newPage() returns a PhabricatorStandardPageView.
  • Add a setFrameable(true) method to that class, following the various other similar options already present (like setShowDurableColumn()).
• In PhabricatorStandardPageView->produceAphrontResponse(), pass setFrameable(true) to the AphrontWebpageResponse if the flag has been set on the PhabricatorStandardPageView.

Then call setFrameable(true) instead of passing 'frame' => true, and the behavior should be preserved.

You can test this in the DarkConsole panel, see Using DarkConsole. Without setFrameable(true), the browser should refuse to load the profile. With the changes above, it should work and look similar to the first screenshot.

You can write "Fixes Txxx" or "Ref Txxx" in the message to automatically link revisions and tasks.

T5132 has a more complete list of syntax. This should be part of the application somehow, but I'm hesitant to just bury it in the documentation. We added tips to arc diff a while ago but I don't think they work very well (they're really easy to miss).

As for installing xhprof, my best guesses are:

• Sometimes the actual "load the extension" stuff doesn't make it to php.ini. You may just need to add this:

extension=xhprof.so

• Sometimes PHP has separate CLI and web config, and it may have been installed to one but not the other. The first ~50 lines of php -i should show you which file is being loaded. That might need an extension=xhprof.so, even if the install process put that line into the web config.
• You may need to restart your webserver for the extension to appear on the web.

Cool, this looks correct to me. Were you able to get XHProf working and actually test it?

You should be able to resolve that unit test failure by running bin/celerity map. Normally, you only have to do this if you've changed CSS or JS, but it may be detecting an out-of-date resource in a third-party library or something. That command should always be safe to run -- it just updates some generated artifacts to reflect the state of CSS and JS on disk.

@epriestley yes I got xhprof up and running and was able to test this successfully. Removing setFrameable(true) did indeed stop it from showing up.

Regarding the test failure, I ran bin/celerity map locally and did not get any failed tests locally.

Oh, it looks like the iframe works but is actually including all the page chrome (header, footer, etc):

This is because 'frame' previously had some other effects in PhabricatorXHProfController->buildStandardPageResponse(). I think you can:

• Remove that method (which no longer has any callers).
• Call setShowChrome(false) and setDisableConsole(true) next to the new setFrameable(true) call, to disable the chrome.

Also, looking into this I think I led you astray a bit here -- PhabricatorBarePageView, the parent class of PhabricatorStandardPageView, already has a setFrameable() method that I missed earlier, so we don't need to add a new one. The new code to pass the flag to AphrontWebpageResponse is correct, but you can remove the new setFrameable() and getFrameable() methods and the property, since they're redundant. Then you'll need to replace $this->frameable with $this->getFrameable().

I'll take a look at the test.

Looks like the test failure is related to D16431, should be an easy fix.