PDF: Fix potential bad indexes in find results.

pdf/pdfium/pdfium_engine.cc

Index: pdf/pdfium/pdfium_engine.cc
diff --git a/pdf/pdfium/pdfium_engine.cc b/pdf/pdfium/pdfium_engine.cc
index 9fb2b7e099bdad4d6829792039df90c94d250e2e..f4eccbb0abb12b2d6d5714395d0dcc35b2b629ea 100644
--- a/pdf/pdfium/pdfium_engine.cc
+++ b/pdf/pdfium/pdfium_engine.cc
@@ -566,8 +566,6 @@ PDFiumEngine::PDFiumEngine(PDFEngine::Client* client)
       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),
@@ -1686,10 +1684,17 @@ void PDFiumEngine::StartFind(const char* text, bool case_sensitive) {
     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);
@@ -1772,26 +1777,29 @@ void PDFiumEngine::SearchUsingICU(const base::string16& term,
 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);
 }
@@ -1805,49 +1813,57 @@ bool PDFiumEngine::SelectFindResult(bool forward) {
   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;
-  } else {
-    if (--current_find_index_ < 0) {
-      current_find_index_ = find_results_.size() - 1;
+  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 {
+    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 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);
-
-    // 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);
+  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.
+    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);
+      }
     }
-  }
 
-  client_->NotifySelectedFindResultChanged(current_find_index_);
+    client_->NotifySelectedFindResultChanged(current_find_index_.GetIndex());
+  }
 
   return true;
 }
@@ -1861,7 +1877,7 @@ void PDFiumEngine::StopFind() {
   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();
@@ -1893,30 +1909,12 @@ void PDFiumEngine::ZoomUpdated(double new_zoom_level) {
 
 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() {
@@ -2809,6 +2807,32 @@ bool PDFiumEngine::MouseDownState::Matches(
   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,
@@ -2988,6 +3012,24 @@ void PDFiumEngine::OnSelectionChanged() {
     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,