Bug 1368408 Selection::SelectFrames() can assume that the result of nsRange::GetStartParent(), nsRange::GetEndParent() and nsIContentIterator::GetCurrentNode() never returns nullptr in it r?mats draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Mon, 29 May 2017 14:25:11 +0900
changeset 585887 f9aa57b2d87ac7ca36e2b52e3f6fe10d1ae7f0c7
parent 585886 ba5369d2b57b175b8b6958604cf61a77fe67158c
child 630822 eb18ccdfd98bf309f3c6e139bf5b23cef5da10bf
push id61220
push usermasayuki@d-toybox.com
push dateMon, 29 May 2017 08:14:29 +0000
reviewersmats
bugs1368408
milestone55.0a1
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);