PDF: Fix potential bad indexes in find results.

pdf/pdfium/pdfium_engine.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "pdf/pdfium/pdfium_engine.h"
 
 #include <math.h>
 
 #include "base/json/json_writer.h"
 #include "base/logging.h"
@@ skipping 548 matching lines @@
       password_tries_remaining_(0),
       doc_(NULL),
       form_(NULL),
       defer_page_unload_(false),
       selecting_(false),
       mouse_down_state_(PDFiumPage::NONSELECTABLE_AREA,
                         PDFiumPage::LinkTarget()),
       next_page_to_search_(-1),
       last_page_to_search_(-1),
       last_character_index_to_search_(-1),
-      current_find_index_(-1),
-      resume_find_index_(-1),
       permissions_(0),
       fpdf_availability_(NULL),
       next_timer_id_(0),
       last_page_mouse_down_(-1),
       first_visible_page_(-1),
       most_visible_page_(-1),
       called_do_document_action_(false),
       render_grayscale_(false),
       progressive_paint_timeout_(0),
       getting_password_(false) {
@@ skipping 1098 matching lines @@
        // Started midway, but only 1 page and we already looped around.
        (pages_.size() == 1 && !first_search) ||
        // Started midway, and we've just looped around.
        (pages_.size() > 1 && current_page == next_page_to_search_));
 
   if (end_of_search) {
     // Send the final notification.
     client_->NotifyNumberOfFindResultsChanged(find_results_.size(), true);
 
     // When searching is complete, resume finding at a particular index.
-    if (resume_find_index_ != -1) {
-      current_find_index_ = resume_find_index_;
-      resume_find_index_ = -1;
+    // Assuming the user has not clicked the find button in the meanwhile.
+    if (resume_find_index_.valid() && !current_find_index_.valid()) {
+      size_t resume_index = resume_find_index_.GetIndex();
+      if (resume_index >= find_results_.size()) {
+        // This might happen if the PDF has some dynamically generated text?
+        resume_index = 0;
+      }
+      current_find_index_.SetIndex(resume_index);
+      client_->NotifySelectedFindResultChanged(resume_index);
     }
+    resume_find_index_.Invalidate();
   } else {
     pp::CompletionCallback callback =
         find_factory_.NewCallback(&PDFiumEngine::ContinueFind);
     pp::Module::Get()->core()->CallOnMainThread(
         0, callback, case_sensitive ? 1 : 0);
   }
 }
 
 void PDFiumEngine::SearchUsingPDFium(const base::string16& term,
                                      bool case_sensitive,
@@ skipping 62 matching lines @@
     int end = FPDFText_GetCharIndexFromTextIndex(
         pages_[current_page]->GetTextPage(),
         temp_start + results[i].length);
     AddFindResult(PDFiumRange(pages_[current_page], start, end - start));
   }
 }
 
 void PDFiumEngine::AddFindResult(const PDFiumRange& result) {
   // Figure out where to insert the new location, since we could have
   // started searching midway and now we wrapped.
-  size_t i;
+  size_t result_index;
   int page_index = result.page_index();
   int char_index = result.char_index();
-  for (i = 0; i < find_results_.size(); ++i) {
-    if (find_results_[i].page_index() > page_index ||
-        (find_results_[i].page_index() == page_index &&
-         find_results_[i].char_index() > char_index)) {
+  for (result_index = 0; result_index < find_results_.size(); ++result_index) {
+    if (find_results_[result_index].page_index() > page_index ||
+        (find_results_[result_index].page_index() == page_index &&
+         find_results_[result_index].char_index() > char_index)) {
       break;
     }
   }
-  find_results_.insert(find_results_.begin() + i, result);
+  find_results_.insert(find_results_.begin() + result_index, result);
   UpdateTickMarks();
 
-  if (current_find_index_ == -1 && resume_find_index_ == -1) {
-    // Select the first match.
+  if (current_find_index_.valid()) {
+    if (result_index <= current_find_index_.GetIndex()) {
+      // Update the current match index
+      size_t find_index = current_find_index_.IncrementIndex();

[raymes, 2014/11/03 05:17:30]

It might be a bit simpler to just have IncrementIndex not return a value.

[Lei Zhang, 2014/11/03 23:25:41]

It's not that much more work.
void IncrementIndex() { ++index_; }
vs
size_t IncrementIndex() { return ++index_; }

+      DCHECK_LT(find_index, find_results_.size());
+      client_->NotifySelectedFindResultChanged(current_find_index_.GetIndex());
+    }
+  } else if (!resume_find_index_.valid()) {
+    // Both indices are invalid. Select the first match.
     SelectFindResult(true);
-  } else if (static_cast<int>(i) <= current_find_index_) {
-    // Update the current match index
-    current_find_index_++;
-    client_->NotifySelectedFindResultChanged(current_find_index_);
   }
   client_->NotifyNumberOfFindResultsChanged(find_results_.size(), false);
 }
 
 bool PDFiumEngine::SelectFindResult(bool forward) {
   if (find_results_.empty()) {
     NOTREACHED();
     return false;
   }
 
   SelectionChangeInvalidator selection_invalidator(this);
 
   // Move back/forward through the search locations we previously found.
-  if (forward) {
-    if (++current_find_index_ == static_cast<int>(find_results_.size()))
-      current_find_index_ = 0;
+  size_t new_index;
+  const size_t last_index = find_results_.size() - 1;
+  if (current_find_index_.valid()) {
+    size_t current_index = current_find_index_.GetIndex();
+    if (forward) {
+      new_index = (current_index >= last_index) ?  0 : current_index + 1;
+    } else {
+      new_index = (current_find_index_.GetIndex() == 0) ?
+          last_index : current_index - 1;
+    }

[raymes, 2014/11/03 05:17:31]

optional: Alternatively I guess you could have Increment(size_t size) and do the
wraparound inside the function (you would need a Decrement too).

[Lei Zhang, 2014/11/03 23:25:42]

The class doesn't know about the size of |find_results_|, so the wrap around
code is staying outside.

   } else {
-    if (--current_find_index_ < 0) {
-      current_find_index_ = find_results_.size() - 1;
-    }
+    new_index = forward ? 0 : last_index;
   }
+  current_find_index_.SetIndex(new_index);
 
   // Update the selection before telling the client to scroll, since it could
   // paint then.
   selection_.clear();
-  selection_.push_back(find_results_[current_find_index_]);
+  if (current_find_index_.valid()) {

[raymes, 2014/11/03 05:17:30]

Since we call SetIndex right above, won't this always be valid?

[Lei Zhang, 2014/11/03 23:25:41]

Indeed it is.

+    selection_.push_back(find_results_[current_find_index_.GetIndex()]);
 
-  // If the result is not in view, scroll to it.
-  size_t i;
-  pp::Rect bounding_rect;
-  pp::Rect visible_rect = GetVisibleRect();
-  // Use zoom of 1.0 since visible_rect is without zoom.
-  std::vector<pp::Rect> rects = find_results_[current_find_index_].
-      GetScreenRects(pp::Point(), 1.0, current_rotation_);
-  for (i = 0; i < rects.size(); ++i)
-    bounding_rect = bounding_rect.Union(rects[i]);
-  if (!visible_rect.Contains(bounding_rect)) {
-    pp::Point center = bounding_rect.CenterPoint();
-    // Make the page centered.
-    int new_y = static_cast<int>(center.y() * current_zoom_) -
-                static_cast<int>(visible_rect.height() * current_zoom_ / 2);
-    if (new_y < 0)
-      new_y = 0;
-    client_->ScrollToY(new_y);
+    // If the result is not in view, scroll to it.
+    pp::Rect bounding_rect;
+    pp::Rect visible_rect = GetVisibleRect();
+    // Use zoom of 1.0 since visible_rect is without zoom.
+    std::vector<pp::Rect> rects;
+    rects = find_results_[current_find_index_.GetIndex()].GetScreenRects(
+        pp::Point(), 1.0, current_rotation_);
+    for (size_t i = 0; i < rects.size(); ++i)
+      bounding_rect = bounding_rect.Union(rects[i]);
+    if (!visible_rect.Contains(bounding_rect)) {
+      pp::Point center = bounding_rect.CenterPoint();
+      // Make the page centered.
+      int new_y = static_cast<int>(center.y() * current_zoom_) -
+          static_cast<int>(visible_rect.height() * current_zoom_ / 2);
+      if (new_y < 0)
+        new_y = 0;
+      client_->ScrollToY(new_y);
 
-    // Only move horizontally if it's not visible.
-    if (center.x() < visible_rect.x() || center.x() > visible_rect.right()) {
-      int new_x = static_cast<int>(center.x()  * current_zoom_) -
-                  static_cast<int>(visible_rect.width() * current_zoom_ / 2);
-      if (new_x < 0)
-        new_x = 0;
-      client_->ScrollToX(new_x);
+      // Only move horizontally if it's not visible.
+      if (center.x() < visible_rect.x() || center.x() > visible_rect.right()) {
+        int new_x = static_cast<int>(center.x()  * current_zoom_) -
+            static_cast<int>(visible_rect.width() * current_zoom_ / 2);
+        if (new_x < 0)
+          new_x = 0;
+        client_->ScrollToX(new_x);
+      }
     }
+
+    client_->NotifySelectedFindResultChanged(current_find_index_.GetIndex());
   }
 
-  client_->NotifySelectedFindResultChanged(current_find_index_);
-
   return true;
 }
 
 void PDFiumEngine::StopFind() {
   SelectionChangeInvalidator selection_invalidator(this);
 
   selection_.clear();
   selecting_ = false;
   find_results_.clear();
   next_page_to_search_ = -1;
   last_page_to_search_ = -1;
   last_character_index_to_search_ = -1;
-  current_find_index_ = -1;
+  current_find_index_.Invalidate();
   current_find_text_.clear();
   UpdateTickMarks();
   find_factory_.CancelAll();
 }
 
 void PDFiumEngine::UpdateTickMarks() {
   std::vector<pp::Rect> tickmarks;
   for (size_t i = 0; i < find_results_.size(); ++i) {
     pp::Rect rect;
     // Always use an origin of 0,0 since scroll positions don't affect tickmark.
@@ skipping 11 matching lines @@
   CancelPaints();
 
   current_zoom_ = new_zoom_level;
 
   CalculateVisiblePages();
   UpdateTickMarks();
 }
 
 void PDFiumEngine::RotateClockwise() {
   current_rotation_ = (current_rotation_ + 1) % 4;
-
-  // Store the current find index so that we can resume finding at that
-  // particular index after we have recomputed the find results.
-  std::string current_find_text = current_find_text_;
-  resume_find_index_ = current_find_index_;
-
-  InvalidateAllPages();
-
-  if (!current_find_text.empty())
-    StartFind(current_find_text.c_str(), false);
+  RotateInternal();
 }
 
 void PDFiumEngine::RotateCounterclockwise() {
   current_rotation_ = (current_rotation_ - 1) % 4;
-
-  // Store the current find index so that we can resume finding at that
-  // particular index after we have recomputed the find results.
-  std::string current_find_text = current_find_text_;
-  resume_find_index_ = current_find_index_;
-
-  InvalidateAllPages();
-
-  if (!current_find_text.empty())
-    StartFind(current_find_text.c_str(), false);
+  RotateInternal();
 }
 
 void PDFiumEngine::InvalidateAllPages() {
   CancelPaints();
   StopFind();
   LoadPageInfo(true);
   client_->Invalidate(pp::Rect(plugin_size_));
 }
 
 std::string PDFiumEngine::GetSelectedText() {
@@ skipping 872 matching lines @@
   if (area_ == area) {
     if (area == PDFiumPage::WEBLINK_AREA)
       return target_.url == target.url;
     if (area == PDFiumPage::DOCLINK_AREA)
       return target_.page == target.page;
     return true;
   }
   return false;
 }
 
+PDFiumEngine::FindTextData::FindTextData()
+    : valid_(false), index_(0) {
+}
+
+PDFiumEngine::FindTextData::~FindTextData() {
+}
+
+void PDFiumEngine::FindTextData::Invalidate() {
+  valid_ = false;
+}
+
+size_t PDFiumEngine::FindTextData::GetIndex() const {
+  DCHECK(valid_);
+  return index_;
+}
+
+void PDFiumEngine::FindTextData::SetIndex(size_t index) {
+  valid_ = true;
+  index_ = index;
+}
+
+size_t PDFiumEngine::FindTextData::IncrementIndex() {
+  DCHECK(valid_);
+  return ++index_;
+}
+
 void PDFiumEngine::DeviceToPage(int page_index,
                                 float device_x,
                                 float device_y,
                                 double* page_x,
                                 double* page_y) {
   *page_x = *page_y = 0;
   int temp_x = static_cast<int>((device_x + position_.x())/ current_zoom_ -
       pages_[page_index]->rect().x());
   int temp_y = static_cast<int>((device_y + position_.y())/ current_zoom_ -
       pages_[page_index]->rect().y());
@@ skipping 159 matching lines @@
   buffer += location.y() * (*stride);
   buffer += (location.x() + page_offset_.x()) * 4;
   *region = buffer;
 }
 
 void PDFiumEngine::OnSelectionChanged() {
   if (HasPermission(PDFEngine::PERMISSION_COPY))
     pp::PDF::SetSelectedText(GetPluginInstance(), GetSelectedText().c_str());
 }
 
+void PDFiumEngine::RotateInternal() {
+  // Store the current find index so that we can resume finding at that
+  // particular index after we have recomputed the find results.
+  std::string current_find_text = current_find_text_;
+  if (current_find_index_.valid())
+    resume_find_index_.SetIndex(current_find_index_.GetIndex());
+  else
+    resume_find_index_.Invalidate();
+
+  InvalidateAllPages();
+
+  if (!current_find_text.empty()) {
+    // Clear the UI.
+    client_->NotifyNumberOfFindResultsChanged(0, false);
+    StartFind(current_find_text.c_str(), false);
+  }
+}
+
 void PDFiumEngine::Form_Invalidate(FPDF_FORMFILLINFO* param,
                                    FPDF_PAGE page,
                                    double left,
                                    double top,
                                    double right,
                                    double bottom) {
   PDFiumEngine* engine = static_cast<PDFiumEngine*>(param);
   int page_index = engine->GetVisiblePageIndex(page);
   if (page_index == -1) {
     // This can sometime happen when the page is closed because it went off
@@ skipping 535 matching lines @@
     double* height) {
   FPDF_DOCUMENT doc = FPDF_LoadMemDocument(pdf_buffer, pdf_buffer_size, NULL);
   if (!doc)
     return false;
   bool success = FPDF_GetPageSizeByIndex(doc, page_number, width, height) != 0;
   FPDF_CloseDocument(doc);
   return success;
 }
 
 }  // namespace chrome_pdf