Bug 1368408 Selection::SelectFrames() can assume that the result of nsRange::GetStartParent(), nsRange::GetEndParent() and nsIContentIterator::GetCurrentNode() never returns nullptr in it r=mats
authorMasayuki Nakano <masayuki@d-toybox.com>
Mon, 29 May 2017 14:25:11 +0900
changeset 409290 0ca74eae6bebe1ddbc4f807e7b20d026a4c40779
parent 409289 7d2efa6434de82c3152a1d02b240dc394ffe62fb
child 409291 c14a7823ce37ca55fc78e0d7e970e8ee764fcd80
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats
bugs1368408
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1368408 Selection::SelectFrames() can assume that the result of nsRange::GetStartParent(), nsRange::GetEndParent() and nsIContentIterator::GetCurrentNode() never returns nullptr in it r=mats Selection::SelectFrames() is an internal method of Selection, it always handles positioned range. So, neither aRange->GetStartParent() nor aRange->GetEndParent() never returns nullptr. Additionally, nsIContentIterator::GetCurrentNode() shouldn't return nullptr until IsDone() returns true. MozReview-Commit-ID: 1IS4nMLukt
layout/generic/nsSelection.cpp
--- a/layout/generic/nsSelection.cpp
+++ b/layout/generic/nsSelection.cpp
@@ -4497,21 +4497,18 @@ Selection::SelectAllFramesForContent(nsI
   }
 
   if (NS_WARN_IF(NS_FAILED(aInnerIter->Init(aContent)))) {
     return NS_ERROR_FAILURE;
   }
 
   for (; !aInnerIter->IsDone(); aInnerIter->Next()) {
     nsINode* node = aInnerIter->GetCurrentNode();
-    // Detect the bug of content iterator, but shouldn't cause a crash in
-    // release builds.
     MOZ_ASSERT(node);
-    nsIContent* innercontent =
-      node && node->IsContent() ? node->AsContent() : nullptr;
+    nsIContent* innercontent = node->IsContent() ? node->AsContent() : nullptr;
     SelectFramesForContent(innercontent, aSelected);
   }
 
   return NS_OK;
 }
 
 /**
  * The idea of this helper method is to select or deselect "top to bottom",
@@ -4520,36 +4517,39 @@ Selection::SelectAllFramesForContent(nsI
 nsresult
 Selection::SelectFrames(nsPresContext* aPresContext, nsRange* aRange,
                         bool aSelect)
 {
   if (!mFrameSelection || !aPresContext || !aPresContext->GetPresShell()) {
     // nothing to do
     return NS_OK;
   }
-  MOZ_ASSERT(aRange);
+  MOZ_ASSERT(aRange && aRange->IsPositioned());
 
   if (mFrameSelection->GetTableCellSelection()) {
     nsINode* node = aRange->GetCommonAncestor();
     nsIFrame* frame = node->IsContent() ? node->AsContent()->GetPrimaryFrame()
                                 : aPresContext->FrameManager()->GetRootFrame();
     if (frame) {
       frame->InvalidateFrameSubtree();
     }
     return NS_OK;
   }
 
 
   // Loop through the content iterator for each content node; for each text
   // node, call SetSelected on it:
   nsINode* startNode = aRange->GetStartParent();
   nsIContent* startContent =
-    startNode && startNode->IsContent() ? startNode->AsContent() : nullptr;
+    startNode->IsContent() ? startNode->AsContent() : nullptr;
   if (!startContent) {
     // Don't warn, bug 1055722
+    // XXX The range can start from a document node and such range can be
+    //     added to Selection with JS.  Therefore, even in such cases,
+    //     shouldn't we handle selection in the range?
     return NS_ERROR_UNEXPECTED;
   }
 
   // We must call first one explicitly
   bool isFirstContentTextNode = startContent->IsNodeOfType(nsINode::eTEXT);
   nsINode* endNode = aRange->GetEndParent();
   if (isFirstContentTextNode) {
     nsIFrame* frame = startContent->GetPrimaryFrame();
@@ -4586,28 +4586,28 @@ Selection::SelectFrames(nsPresContext* a
   nsCOMPtr<nsIContentIterator> iter = NS_NewContentSubtreeIterator();
   iter->Init(aRange);
   if (isFirstContentTextNode && !iter->IsDone()) {
     iter->Next(); // first content has already been handled.
   }
   nsCOMPtr<nsIContentIterator> inneriter = NS_NewContentIterator();
   for (; !iter->IsDone(); iter->Next()) {
     nsINode* node = iter->GetCurrentNode();
-    // Detect the bug of content iterator, but shouldn't cause a crash in
-    // release builds.
     MOZ_ASSERT(node);
-    nsIContent* content =
-      node && node->IsContent() ? node->AsContent() : nullptr;
+    nsIContent* content = node->IsContent() ? node->AsContent() : nullptr;
     SelectAllFramesForContent(inneriter, content, aSelect);
   }
 
   // We must now do the last one if it is not the same as the first
   if (endNode != startNode) {
     nsIContent* endContent =
-      endNode && endNode->IsContent() ? endNode->AsContent() : nullptr;
+      endNode->IsContent() ? endNode->AsContent() : nullptr;
+    // XXX The range can end at a document node and such range can be
+    //     added to Selection with JS.  Therefore, even in such cases,
+    //     shouldn't we handle selection in the range?
     if (NS_WARN_IF(!endContent)) {
       return NS_ERROR_UNEXPECTED;
     }
     if (endContent->IsNodeOfType(nsINode::eTEXT)) {
       nsIFrame* frame = endContent->GetPrimaryFrame();
       // The frame could be an SVG text frame, in which case we'll ignore it.
       if (frame && frame->IsTextFrame()) {
         nsTextFrame* textFrame = static_cast<nsTextFrame*>(frame);