When proxying an "{image ...}" image fails, show the user an error message

Summary:
Depends on D19192. Ref T4190. Ref T13101. Instead of directly including the proxy endpoint with `<img src="..." />`, emit a placeholder and use AJAX to make the request. If the proxy fetch fails, replace the placeholder with an error message.

This isn't the most polished implementation imaginable, but it's much less mysterious about errors.

Test Plan: Used `{image ...}` for valid and invalid images, got images and useful error messages respectively.

Maniphest Tasks: T13101, T4190

Differential Revision: https://secure.phabricator.com/D19193

resources/celerity/map.php

@@ -9,7 +9,7 @@ return array(
   'names' => array(
     'conpherence.pkg.css' => 'e68cf1fa',
     'conpherence.pkg.js' => '15191c65',
-    'core.pkg.css' => '2fa91e14',
+    'core.pkg.css' => 'c218ed53',
     'core.pkg.js' => '32bb68e9',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '113e692c',
@@ -114,7 +114,7 @@ return array(
     'rsrc/css/application/tokens/tokens.css' => '3d0f239e',
     'rsrc/css/application/uiexample/example.css' => '528b19de',
     'rsrc/css/core/core.css' => '62fa3ace',
-    'rsrc/css/core/remarkup.css' => 'cad18339',
+    'rsrc/css/core/remarkup.css' => '97dc3523',
     'rsrc/css/core/syntax.css' => 'cae95e89',
     'rsrc/css/core/z-index.css' => '9d8f7c4b',
     'rsrc/css/diviner/diviner-shared.css' => '896f1d43',
@@ -504,6 +504,7 @@ return array(
     'rsrc/js/core/behavior-read-only-warning.js' => 'ba158207',
     'rsrc/js/core/behavior-redirect.js' => '0213259f',
     'rsrc/js/core/behavior-refresh-csrf.js' => 'ab2f381b',
+    'rsrc/js/core/behavior-remarkup-load-image.js' => '040fce04',
     'rsrc/js/core/behavior-remarkup-preview.js' => '4b700e9e',
     'rsrc/js/core/behavior-reorder-applications.js' => '76b9fc3e',
     'rsrc/js/core/behavior-reveal-content.js' => '60821bc7',
@@ -692,6 +693,7 @@ return array(
     'javelin-behavior-releeph-preview-branch' => 'b2b4fbaf',
     'javelin-behavior-releeph-request-state-change' => 'a0b57eb8',
     'javelin-behavior-releeph-request-typeahead' => 'de2e896f',
+    'javelin-behavior-remarkup-load-image' => '040fce04',
     'javelin-behavior-remarkup-preview' => '4b700e9e',
     'javelin-behavior-reorder-applications' => '76b9fc3e',
     'javelin-behavior-reorder-columns' => 'e1d25dfb',
@@ -800,7 +802,7 @@ return array(
     'phabricator-object-selector-css' => '85ee8ce6',
     'phabricator-phtize' => 'd254d646',
     'phabricator-prefab' => '77b0ae28',
-    'phabricator-remarkup-css' => 'cad18339',
+    'phabricator-remarkup-css' => '97dc3523',
     'phabricator-search-results-css' => '505dd8cf',
     'phabricator-shaped-request' => '7cbe244b',
     'phabricator-slowvote-css' => 'a94b7230',
@@ -940,6 +942,10 @@ return array(
       'javelin-behavior',
       'javelin-uri',
     ),
+    '040fce04' => array(
+      'javelin-behavior',
+      'javelin-request',
+    ),
     '04b2ae03' => array(
       'javelin-install',
       'javelin-util',

src/__phutil_library_map__.php

@@ -1883,6 +1883,7 @@ phutil_register_library_map(array(
     'PHUIPropertyGroupView' => 'view/phui/PHUIPropertyGroupView.php',
     'PHUIPropertyListExample' => 'applications/uiexample/examples/PHUIPropertyListExample.php',
     'PHUIPropertyListView' => 'view/phui/PHUIPropertyListView.php',
+    'PHUIRemarkupImageView' => 'infrastructure/markup/view/PHUIRemarkupImageView.php',
     'PHUIRemarkupPreviewPanel' => 'view/phui/PHUIRemarkupPreviewPanel.php',
     'PHUIRemarkupView' => 'infrastructure/markup/view/PHUIRemarkupView.php',
     'PHUISegmentBarSegmentView' => 'view/phui/PHUISegmentBarSegmentView.php',
@@ -7287,6 +7288,7 @@ phutil_register_library_map(array(
     'PHUIPropertyGroupView' => 'AphrontTagView',
     'PHUIPropertyListExample' => 'PhabricatorUIExample',
     'PHUIPropertyListView' => 'AphrontView',
+    'PHUIRemarkupImageView' => 'AphrontView',
     'PHUIRemarkupPreviewPanel' => 'AphrontTagView',
     'PHUIRemarkupView' => 'AphrontView',
     'PHUISegmentBarSegmentView' => 'AphrontTagView',

src/applications/files/controller/PhabricatorFileImageProxyController.php

@@ -44,6 +44,7 @@ final class PhabricatorFileImageProxyController
       ->setTTL($ttl);
 
     $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+    $save_request = false;
     // Cache missed so we'll need to validate and download the image
     try {
       // Rate limit outbound fetches to make this mechanism less useful for
@@ -75,44 +76,74 @@ final class PhabricatorFileImageProxyController
         $file->save();
       }
 
-      $external_request->setIsSuccessful(true)
+      $external_request
-        ->setFilePHID($file->getPHID())
+        ->setIsSuccessful(1)
-        ->save();
+        ->setFilePHID($file->getPHID());
-      unset($unguarded);
+
-      return $this->getExternalResponse($external_request);
+      $save_request = true;
     } catch (HTTPFutureHTTPResponseStatus $status) {
-      $external_request->setIsSuccessful(false)
+      $external_request
+        ->setIsSuccessful(0)
         ->setResponseMessage($status->getMessage())
         ->save();
-      return $this->getExternalResponse($external_request);
+
+      $save_request = true;
     } catch (Exception $ex) {
       // Not actually saving the request in this case
       $external_request->setResponseMessage($ex->getMessage());
-      return $this->getExternalResponse($external_request);
     }
+
+    if ($save_request) {
+      try {
+        $external_request->save();
+      } catch (AphrontDuplicateKeyQueryException $ex) {
+        // We may have raced against another identical request. If we did,
+        // just throw our result away and use the winner's result.
+        $external_request = $external_request->loadOneWhere(
+          'uriIndex = %s',
+          PhabricatorHash::digestForIndex($img_uri));
+        if (!$external_request) {
+          throw new Exception(
+            pht(
+              'Hit duplicate key collision when saving proxied image, but '.
+              'failed to load duplicate row (for URI "%s").',
+              $img_uri));
+        }
+      }
+    }
+
+    unset($unguarded);
+
+
+    return $this->getExternalResponse($external_request);
   }
 
   private function getExternalResponse(
     PhabricatorFileExternalRequest $request) {
-    if ($request->getIsSuccessful()) {
+    if (!$request->getIsSuccessful()) {
+      throw new Exception(
+        pht(
+          'Request to "%s" failed: %s',
+          $request->getURI(),
+          $request->getResponseMessage()));
+    }
+
     $file = id(new PhabricatorFileQuery())
       ->setViewer(PhabricatorUser::getOmnipotentUser())
       ->withPHIDs(array($request->getFilePHID()))
       ->executeOne();
     if (!$file) {
-        throw new Exception(pht(
+      throw new Exception(
+        pht(
           'The underlying file does not exist, but the cached request was '.
-          'successful. This likely means the file record was manually deleted '.
+          'successful. This likely means the file record was manually '.
-          'by an administrator.'));
+          'deleted by an administrator.'));
-      }
-      return id(new AphrontRedirectResponse())
-        ->setIsExternal(true)
-        ->setURI($file->getViewURI());
-    } else {
-      throw new Exception(pht(
-        "The request to get the external file from '%s' was unsuccessful:\n %s",
-        $request->getURI(),
-        $request->getResponseMessage()));
     }
+
+    return id(new AphrontAjaxResponse())
+      ->setContent(
+        array(
+          'imageURI' => $file->getViewURI(),
+        ));
   }
 }

src/applications/files/markup/PhabricatorImageRemarkupRule.php

@@ -100,14 +100,11 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule {
       $src_uri = id(new PhutilURI('/file/imageproxy/'))
         ->setQueryParam('uri', $args['uri']);
 
-      $img = $this->newTag(
+      $img = id(new PHUIRemarkupImageView())
-        'img',
+        ->setURI($src_uri)
-        array(
+        ->setAlt($args['alt'])
-          'src' => $src_uri,
+        ->setWidth($args['width'])
-          'alt' => $args['alt'],
+        ->setHeight($args['height']);
-          'width' => $args['width'],
-          'height' => $args['height'],
-        ));
 
       $engine->overwriteStoredText($image['token'], $img);
     }

src/infrastructure/markup/view/PHUIRemarkupImageView.php

@@ -0,0 +1,67 @@
+<?php
+
+final class PHUIRemarkupImageView
+  extends AphrontView {
+
+  private $uri;
+  private $width;
+  private $height;
+  private $alt;
+
+  public function setURI($uri) {
+    $this->uri = $uri;
+    return $this;
+  }
+
+  public function getURI() {
+    return $this->uri;
+  }
+
+  public function setWidth($width) {
+    $this->width = $width;
+    return $this;
+  }
+
+  public function getWidth() {
+    return $this->width;
+  }
+
+  public function setHeight($height) {
+    $this->height = $height;
+    return $this;
+  }
+
+  public function getHeight() {
+    return $this->height;
+  }
+
+  public function setAlt($alt) {
+    $this->alt = $alt;
+    return $this;
+  }
+
+  public function getAlt() {
+    return $this->alt;
+  }
+
+  public function render() {
+    $id = celerity_generate_unique_node_id();
+
+    Javelin::initBehavior(
+      'remarkup-load-image',
+      array(
+        'uri' => (string)$this->uri,
+        'imageID' => $id,
+      ));
+
+    return phutil_tag(
+      'img',
+      array(
+        'id' => $id,
+        'width' => $this->getWidth(),
+        'height' => $this->getHeight(),
+        'alt' => $this->getAlt(),
+      ));
+  }
+
+}

webroot/rsrc/css/core/remarkup.css

@@ -472,6 +472,13 @@ video.phabricator-media {
   margin: .5em 1em 0;
 }
 
+.phabricator-remarkup-image-error {
+  border: 1px solid {$redborder};
+  background: {$sh-redbackground};
+  padding: 8px 12px;
+  color: {$darkgreytext};
+}
+
 .phabricator-remarkup-embed-image {
   display: inline-block;
   border: 3px solid white;

webroot/rsrc/js/core/behavior-remarkup-load-image.js

@@ -0,0 +1,45 @@
+/**
+ * @provides javelin-behavior-remarkup-load-image
+ * @requires javelin-behavior
+ *           javelin-request
+ */
+
+JX.behavior('remarkup-load-image', function(config) {
+
+  function get_node() {
+    try {
+      return JX.$(config.imageID);
+    } catch (ex) {
+      return null;
+    }
+  }
+
+  function onload(r) {
+    var node = get_node();
+    if (!node) {
+      return;
+    }
+
+    node.src = r.imageURI;
+  }
+
+  function onerror(r) {
+    var node = get_node();
+    if (!node) {
+      return;
+    }
+
+    var error = JX.$N(
+      'div',
+      {
+        className: 'phabricator-remarkup-image-error'
+      },
+      r.info);
+
+    JX.DOM.replace(node, error);
+  }
+
+  var request = new JX.Request(config.uri, onload);
+  request.listen('error', onerror);
+  request.send();
+});