PDF: Add a bunch of DCHECKs to make sure we do not go out of bounds.

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 1034 matching lines @@
 
 void PDFiumEngine::PrePaint() {
   for (size_t i = 0; i < progressive_paints_.size(); ++i)
     progressive_paints_[i].painted_ = false;
 }
 
 void PDFiumEngine::Paint(const pp::Rect& rect,
                          pp::ImageData* image_data,
                          std::vector<pp::Rect>* ready,
                          std::vector<pp::Rect>* pending) {
+  DCHECK(image_data);
+  DCHECK(ready);
+  DCHECK(pending);

[raymes, 2015/01/22 22:50:03]

Are these actually useful? I've heard various opinions but some people say that
if something is NULL when it's not supposed to be then there is no real
advantage to adding a DCHECK as a crash with a useful error should appear on the
line where it is first accessed.

With that said, I'm ok to add these iyl.

[Lei Zhang, 2015/01/22 23:00:09]

In the case of:

DCHECK(ptr_var);
ptr_var->Method();

I would also just let it crash and omit the DCHECK(). When put at the top of the
file, they serve as a sanity check and as a form of documentation. If I see this
at the top of a function, I would know/expect the function to use the pointers
without checking if they are null.

+
   pp::Rect leftover = rect;
   for (size_t i = 0; i < visible_pages_.size(); ++i) {
     int index = visible_pages_[i];
     pp::Rect page_rect = pages_[index]->rect();
     // Convert the current page's rectangle to screen rectangle.  We do this
     // instead of the reverse (converting the dirty rectangle from screen to
     // page coordinates) because then we'd have to convert back to screen
     // coordinates, and the rounding errors sometime leave pixels dirty or even
     // move the text up or down a pixel when zoomed.
     pp::Rect page_rect_in_screen = GetPageScreenRect(index);
     pp::Rect dirty_in_screen = page_rect_in_screen.Intersect(leftover);
     if (dirty_in_screen.IsEmpty())
       continue;
 
     leftover = leftover.Subtract(dirty_in_screen);
 
     if (pages_[index]->available()) {
       int progressive = GetProgressiveIndex(index);
-      if (progressive != -1 &&
-          progressive_paints_[progressive].rect != dirty_in_screen) {
-        // The PDFium code can only handle one progressive paint at a time, so
-        // queue this up. Previously we used to merge the rects when this
-        // happened, but it made scrolling up on complex PDFs very slow since
-        // there would be a damaged rect at the top (from scroll) and at the
-        // bottom (from toolbar).
-        pending->push_back(dirty_in_screen);
-        continue;
+      if (progressive != -1) {
+        DCHECK_GE(progressive, 0);
+        DCHECK_LT(static_cast<size_t>(progressive), progressive_paints_.size());
+        if (progressive_paints_[progressive].rect != dirty_in_screen) {
+          // The PDFium code can only handle one progressive paint at a time, so
+          // queue this up. Previously we used to merge the rects when this
+          // happened, but it made scrolling up on complex PDFs very slow since
+          // there would be a damaged rect at the top (from scroll) and at the
+          // bottom (from toolbar).
+          pending->push_back(dirty_in_screen);
+          continue;
+        }
       }
 
       if (progressive == -1) {

[raymes, 2015/01/22 22:50:03]

can we make this an "else" branch of the previous if?

[Lei Zhang, 2015/01/22 23:00:09]

I thought about it, but I can also see the argument that this is the "next step"
so it should be separate.

         progressive = StartPaint(index, dirty_in_screen);
         progressive_paint_timeout_ = kMaxInitialProgressivePaintTimeMs;
       } else {
         progressive_paint_timeout_ = kMaxProgressivePaintTimeMs;

[raymes, 2015/01/22 22:50:03]

Can this go into the previous if-statement?

       }
 
       progressive_paints_[progressive].painted_ = true;
       if (ContinuePaint(progressive, image_data)) {
         FinishPaint(progressive, image_data);
         ready->push_back(dirty_in_screen);
       } else {
         pending->push_back(dirty_in_screen);
       }
     } else {
@@ skipping 1235 matching lines @@
       return (permissions_ & kPDFPermissionCopyMask) != 0;
     case PERMISSION_COPY_ACCESSIBLE:
       return (permissions_ & kPDFPermissionCopyAccessibleMask) != 0;
     case PERMISSION_PRINT_LOW_QUALITY:
       return (permissions_ & kPDFPermissionPrintLowQualityMask) != 0;
     case PERMISSION_PRINT_HIGH_QUALITY:
       return (permissions_ & kPDFPermissionPrintLowQualityMask) != 0 &&
              (permissions_ & kPDFPermissionPrintHighQualityMask) != 0;
     default:
       return true;
-  };
+  }
 }
 
 void PDFiumEngine::SelectAll() {
   SelectionChangeInvalidator selection_invalidator(this);
 
   selection_.clear();
   for (size_t i = 0; i < pages_.size(); ++i)
     if (pages_[i]->available()) {
       selection_.push_back(PDFiumRange(pages_[i], 0,
                            pages_[i]->GetCharCount()));
@@ skipping 476 matching lines @@
   progressive.rect = dirty;
   progressive.page_index = page_index;
   progressive.bitmap = NULL;
   progressive.painted_ = false;
   progressive_paints_.push_back(progressive);
   return progressive_paints_.size() - 1;
 }
 
 bool PDFiumEngine::ContinuePaint(int progressive_index,
                                  pp::ImageData* image_data) {
+  DCHECK_GE(progressive_index, 0);
+  DCHECK_LT(static_cast<size_t>(progressive_index), progressive_paints_.size());
+  DCHECK(image_data);
+
 #if defined(OS_LINUX)
   g_last_instance_id = client_->GetPluginInstance()->pp_instance();
 #endif
 
   int rv;
+  FPDF_BITMAP bitmap = progressive_paints_[progressive_index].bitmap;
   int page_index = progressive_paints_[progressive_index].page_index;
+  DCHECK_GE(page_index, 0);
+  DCHECK_LT(static_cast<size_t>(page_index), pages_.size());
+  FPDF_PAGE page = pages_[page_index]->GetPage();
+
   last_progressive_start_time_ = base::Time::Now();
-  if (progressive_paints_[progressive_index].bitmap) {
-    rv = FPDF_RenderPage_Continue(
-        pages_[page_index]->GetPage(), static_cast<IFSDK_PAUSE*>(this));
+  if (bitmap) {
+    rv = FPDF_RenderPage_Continue(page, static_cast<IFSDK_PAUSE*>(this));
   } else {
     pp::Rect dirty = progressive_paints_[progressive_index].rect;
-    progressive_paints_[progressive_index].bitmap = CreateBitmap(dirty,
-                                                                 image_data);
+    bitmap = CreateBitmap(dirty, image_data);
     int start_x, start_y, size_x, size_y;
-    GetPDFiumRect(
-        page_index, dirty, &start_x, &start_y, &size_x, &size_y);
-    FPDFBitmap_FillRect(progressive_paints_[progressive_index].bitmap, start_x,
-                        start_y, size_x, size_y, 0xFFFFFFFF);
+    GetPDFiumRect(page_index, dirty, &start_x, &start_y, &size_x, &size_y);
+    FPDFBitmap_FillRect(bitmap, start_x, start_y, size_x, size_y, 0xFFFFFFFF);
     rv = FPDF_RenderPageBitmap_Start(
-        progressive_paints_[progressive_index].bitmap,
-        pages_[page_index]->GetPage(), start_x, start_y, size_x, size_y,
+        bitmap, page, start_x, start_y, size_x, size_y,
         current_rotation_,
         GetRenderingFlags(), static_cast<IFSDK_PAUSE*>(this));
+    progressive_paints_[progressive_index].bitmap = bitmap;
   }
   return rv != FPDF_RENDER_TOBECOUNTINUED;
 }
 
 void PDFiumEngine::FinishPaint(int progressive_index,
                                pp::ImageData* image_data) {
+  DCHECK_GE(progressive_index, 0);
+  DCHECK_LT(static_cast<size_t>(progressive_index), progressive_paints_.size());
+  DCHECK(image_data);
+
   int page_index = progressive_paints_[progressive_index].page_index;
   pp::Rect dirty_in_screen = progressive_paints_[progressive_index].rect;
   FPDF_BITMAP bitmap = progressive_paints_[progressive_index].bitmap;
   int start_x, start_y, size_x, size_y;
   GetPDFiumRect(
       page_index, dirty_in_screen, &start_x, &start_y, &size_x, &size_y);
 
   // Draw the forms.
   FPDF_FFLDraw(
       form_, bitmap, pages_[page_index]->GetPage(), start_x, start_y, size_x,
@@ skipping 15 matching lines @@
 
 void PDFiumEngine::CancelPaints() {
   for (size_t i = 0; i < progressive_paints_.size(); ++i) {
     FPDF_RenderPage_Close(pages_[progressive_paints_[i].page_index]->GetPage());
     FPDFBitmap_Destroy(progressive_paints_[i].bitmap);
   }
   progressive_paints_.clear();
 }
 
 void PDFiumEngine::FillPageSides(int progressive_index) {
+  DCHECK_GE(progressive_index, 0);
+  DCHECK_LT(static_cast<size_t>(progressive_index), progressive_paints_.size());
+
   int page_index = progressive_paints_[progressive_index].page_index;
   pp::Rect dirty_in_screen = progressive_paints_[progressive_index].rect;
   FPDF_BITMAP bitmap = progressive_paints_[progressive_index].bitmap;
 
   pp::Rect page_rect = pages_[page_index]->rect();
   if (page_rect.x() > 0) {
     pp::Rect left(0,
                   page_rect.y() - kPageShadowTop,
                   page_rect.x() - kPageShadowLeft,
                   page_rect.height() + kPageShadowTop +
@@ skipping 26 matching lines @@
                   kPageSeparatorThickness);
   bottom = GetScreenRect(bottom).Intersect(dirty_in_screen);
 
   FPDFBitmap_FillRect(bitmap, bottom.x() - dirty_in_screen.x(),
                       bottom.y() - dirty_in_screen.y(), bottom.width(),
                       bottom.height(), kBackgroundColor);
 }
 
 void PDFiumEngine::PaintPageShadow(int progressive_index,
                                    pp::ImageData* image_data) {
+  DCHECK_GE(progressive_index, 0);
+  DCHECK_LT(static_cast<size_t>(progressive_index), progressive_paints_.size());
+  DCHECK(image_data);
+
   int page_index = progressive_paints_[progressive_index].page_index;
   pp::Rect dirty_in_screen = progressive_paints_[progressive_index].rect;
   pp::Rect page_rect = pages_[page_index]->rect();
   pp::Rect shadow_rect(page_rect);
   shadow_rect.Inset(-kPageShadowLeft, -kPageShadowTop,
                     -kPageShadowRight, -kPageShadowBottom);
 
   // Due to the rounding errors of the GetScreenRect it is possible to get
   // different size shadows on the left and right sides even they are defined
   // the same. To fix this issue let's calculate shadow rect and then shrink
   // it by the size of the shadows.
   shadow_rect = GetScreenRect(shadow_rect);
   page_rect = shadow_rect;
 
   page_rect.Inset(static_cast<int>(ceil(kPageShadowLeft * current_zoom_)),
                   static_cast<int>(ceil(kPageShadowTop * current_zoom_)),
                   static_cast<int>(ceil(kPageShadowRight * current_zoom_)),
                   static_cast<int>(ceil(kPageShadowBottom * current_zoom_)));
 
   DrawPageShadow(page_rect, shadow_rect, dirty_in_screen, image_data);
 }
 
 void PDFiumEngine::DrawSelections(int progressive_index,
                                   pp::ImageData* image_data) {
+  DCHECK_GE(progressive_index, 0);
+  DCHECK_LT(static_cast<size_t>(progressive_index), progressive_paints_.size());
+  DCHECK(image_data);
+
   int page_index = progressive_paints_[progressive_index].page_index;
   pp::Rect dirty_in_screen = progressive_paints_[progressive_index].rect;
 
   void* region = NULL;
   int stride;
   GetRegion(dirty_in_screen.point(), image_data, &region, &stride);
 
   std::vector<pp::Rect> highlighted_rects;
   pp::Rect visible_rect = GetVisibleRect();
   for (size_t k = 0; k < selection_.size(); ++k) {
@@ skipping 1005 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