Page MenuHomePhabricator

Removing deprecated method calls
ClosedPublic

Authored by jcox on Aug 23 2016, 2:48 PM.

Details

Summary

Removed call to the deprecated buildStandardPageResponse method from XHProfProfileController

Test Plan

Install, configure, and use XHProf. I'll need some guidance with this

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jcox retitled this revision from to Removing deprecated method calls.
jcox updated this object.
jcox edited the test plan for this revision. (Show Details)

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) ?

src/applications/xhprof/controller/PhabricatorXHProfProfileController.php
54

Does this need to be reflected in the new code somehow? Or is this info passed in with the $view?

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.
jcox edited edge metadata.
  • Revert "Removedj deprecated buildApplicationPage method from PhabricatorController"
  • Added setFrameable method to PhabricatorStandardPageView

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.

jcox marked an inline comment as done.Aug 23 2016, 9:22 PM

@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().

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

  • Removed the now unused method from XHProfController

Also disabled the chrome and console on the embedded frame to prevent super nesting of profile frames

  • Remove the frameable property from StandardPageView
epriestley added a reviewer: epriestley.

Great, thanks!

This exposed an unrelated issue with file encryption, the profiler, DarkConsole, UTF8 sanitization and JSON encoding interacting with one another for me locally so it took a minute for me to dig into that, I'll get a separate diff out to clean that up.

The test failure wasn't your fault and should also be fixed by D16433. You can rebase this on master and re-diff it if you want to get a clean bill of health, or it's fine to just land it.

This revision is now accepted and ready to land.Aug 24 2016, 2:02 PM
jcox edited edge metadata.
  • Revert "Removedj deprecated buildApplicationPage method from PhabricatorController"
  • Added setFrameable method to PhabricatorStandardPageView
  • Removed the now unused method from XHProfController
  • Remove the frameable property from StandardPageView
This revision was automatically updated to reflect the committed changes.