r97851 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r97850‎ | r97851 | r97852 >
Date:20:44, 22 September 2011
Author:vyznev
Status:reverted (Comments)
Tags:
Comment:
followup r79862: the for loop only cleans up half the output handlers (since $i counts up while ob_get_level() counts down); check the return value of ob_end_clean() instead.
(I just noticed this while eyeballing the code -- apparently most people don't have multiple output handlers active, given that nobody had caught this in over eight months.)
Modified paths:
  • /trunk/phase3/includes/resourceloader/ResourceLoader.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/resourceloader/ResourceLoader.php
——@@ -462,9 +462,10 @@
463463 // On some setups, ob_get_level() doesn't seem to go down to zero
464464 // no matter how often we call ob_get_clean(), so instead of doing
465465 // the more intuitive while ( ob_get_level() > 0 ) ob_get_clean();
466 - // we have to be safe here and avoid an infinite loop.
467 - for ( $i = 0; $i < ob_get_level(); $i++ ) {
468 - ob_end_clean();
 466+ // we have to be safe here and check the return value to avoid an
 467+ // infinite loop. (bug 26370)
 468+ while ( ob_get_level() > 0 && ob_end_clean() ) {
 469+ // repeat
469470 }
470471
471472 header( 'HTTP/1.0 304 Not Modified' );

Follow-up revisions

RevisionCommit summaryAuthorDate
r97855rv r97851 for further inspection, causes PHP noticesnikerabbit21:27, 22 September 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r79862Attempt at fixing bug 26370, which seems to be an infinite loop caused by ob_...catrope15:41, 8 January 2011

Comments

#Comment by Nikerabbit (talk | contribs)   21:19, 22 September 2011

[22-Sep-2011 21:18:03] PHP Notice: ob_end_clean(): failed to delete buffer zlib output compression. in /www/w/includes/resourceloader/ResourceLoader.php on line 467

#Comment by Siebrand (talk | contribs)   21:22, 22 September 2011

Niklas is reverting.

#Comment by Ilmari Karonen (talk | contribs)   02:56, 23 September 2011

It turns out that the whole loop could and probably should be replaced by a call to wfClearOutputBuffers(). Oh, and it should probably do ini_set( 'zlib.output_compression', 0 ) too.

Ps. I tried looking a bit deeper, and found what looks like a real mess. Apparently there are at least three slightly different and incompatible ways to gzip-compress the output of MediaWiki (zlib.output_compression, ob_gzhandler and MediaWiki's own wfOutputHandler) and what seem to be the ancient remnants of a fourth ($wgUseGzip). It also seems that they all suffer (or at least used to suffer in some PHP versions) of similar bugs that require disabling them before an HTTP 304 response is sent. wfClearOutputBuffers() correctly does that for two of them, but not for zlib.output_compression, which needs a separate ini_set() call. Oh, and there's a whole bunch of places in MediaWiki that send 304 responses, and currently only one of them (OutputPage::checkLastModified()) seems to handle all the cases right. Eurgh.

Some references/bookmarks: bug 8148 / r18253, bug 26130 / r78199, bug 26370 / r79862.

Status & tagging log