Bug 1677194 Comment 7 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

Original comment by
on 
So the problematic pointer value was used to read 4 bytes at
       0x625001200e44.
BTW, at the time of the error.
this = 0x625001200d10

The memory area was allocated by the following chain of functions, and it was 8KB at the allocation time.
(Too bad, ASAN doesn't say when the area was poisoned after free?)
although the area is very close to the area pointed at by |this|,
it may not be related. I am not sure.

The attached log was produced by the following patch.
```
diff --git a/layout/xul/tree/nsTreeBodyFrame.cpp b/layout/xul/tree/nsTreeBodyFrame.cpp
--- a/layout/xul/tree/nsTreeBodyFrame.cpp
+++ b/layout/xul/tree/nsTreeBodyFrame.cpp
@@ -1593,20 +1593,36 @@ nsresult nsTreeBodyFrame::RowCountChange
   if (aCount == 0 || !mView) return NS_OK;  // Nothing to do.
 
 #ifdef ACCESSIBILITY
   if (PresShell::IsAccessibilityActive()) {
     FireRowCountChangedEvent(aIndex, aCount);
   }
 #endif  // #ifdef ACCESSIBILITY
 
-  // Adjust our selection.
-  nsCOMPtr<nsITreeSelection> sel;
-  mView->GetSelection(getter_AddRefs(sel));
-  if (sel) sel->AdjustSelection(aIndex, aCount);
+  // Validate pointer. Bug 1677194
+  AutoWeakFrame weakFrame(this);
+  nsCOMPtr<nsITreeView> view = mView;
+
+  // Adjust our selection. Note the use of |view| instead of |mView| 
+  nsCOMPtr<nsITreeSelection> sel = nullptr; 
+#if DEBUG
+  printf("RowCountChanged(): view = %p, sel = %p, this = %p\n", (void *) view, (void *) sel, (void *) this);
+  fflush(stdout);
+#endif
+  nsresult rc = view->GetSelection(getter_AddRefs(sel));
+  // Check
+  NS_ENSURE_STATE(weakFrame.IsAlive());
+
+#if DEBUG
+  printf("RowCountChanged(): view = %p, sel = %p, this = %p\n", (void *) view, (void *) sel, (void *) this);
+  fflush(stdout);
+#endif
+
+  if (NS_SUCCEEDED(rc) && sel) sel->AdjustSelection(aIndex, aCount);
 
   if (mUpdateBatchNest) return NS_OK;
 
   mRowCount += aCount;
 #ifdef DEBUG
   int32_t rowCount = mRowCount;
   mView->GetRowCount(&rowCount);
   NS_ASSERTION(

```
Revision 1 by
on 
So the problematic pointer value was used to read 4 bytes at
       0x625001200e44.
BTW, at the time of the error.
this = 0x625001200d10

The memory area was allocated by the following chain of functions, and it was 8KB at the allocation time.
(Too bad, ASAN doesn't say when the area was poisoned after free?)
although the area is very close to the area pointed at by |this|,
it may not be related. I am not sure.
```

 1:50.87 GECKO(518898) 0x625001200e44 is located 5444 bytes inside of 8192-byte region [0x6250011ff900,0x625001201900)
 1:50.87 GECKO(518898) allocated by thread T0 here:
 1:50.93 GECKO(518898)     #0 0x55a5d5f1376d in malloc (/NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/bin/thunderbird+0x16176d)
 1:50.93 GECKO(518898)     #1 0x7f44b8ff2728 in AllocateChunk /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/mozilla/ArenaAllocator.h:171:15
 1:50.93 GECKO(518898)     #2 0x7f44b8ff2728 in InternalAllocate /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/mozilla/ArenaAllocator.h:205:25
 1:50.93 GECKO(518898)     #3 0x7f44b8ff2728 in mozilla::ArenaAllocator<8192ul, 8ul>::Allocate(unsigned long, std::nothrow_t const&) /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/mozilla/ArenaAllocator.h:67:12
 1:50.93 GECKO(518898)     #4 0x7f44c5ccf2c7 in Allocate /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/mozilla/ArenaAllocator.h:71:15
 1:50.93 GECKO(518898)     #5 0x7f44c5ccf2c7 in nsPresArena<8192ul, mozilla::ArenaObjectID, 176ul>::Allocate(mozilla::ArenaObjectID, unsigned long) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/base/nsPresArena.cpp:100:16
 1:50.93 GECKO(518898)     #6 0x7f44c670c13a in AllocateByObjectID /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/mozilla/PresShell.h:260:32
 1:50.93 GECKO(518898)     #7 0x7f44c670c13a in operator new /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/painting/FrameLayerBuilder.h:103:39
 1:50.93 GECKO(518898)     #8 0x7f44c670c13a in mozilla::FrameLayerBuilder::StoreDataForFrame(nsPaintedDisplayItem*, mozilla::layers::Layer*, mozilla::LayerState, mozilla::DisplayItemData*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/painting/FrameLayerBuilder.cpp:5504:34
 1:50.93 GECKO(518898)     #9 0x7f44c670de1b in mozilla::FrameLayerBuilder::AddPaintedDisplayItem(mozilla::PaintedLayerData*, mozilla::AssignedDisplayItem&, mozilla::layers::Layer*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/painting/FrameLayerBuilder.cpp:5370:14
 1:50.93 GECKO(518898)     #10 0x7f44c66dce8d in FinishPaintedLayerData<(lambda at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/painting/FrameLayerBuilder.cpp:3144:52)> /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/painting/FrameLayerBuilder.cpp:3501:20
 1:50.93 GECKO(518898)     #11 0x7f44c66dce8d in mozilla::PaintedLayerDataNode::PopAllPaintedLayerData() /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/painting/FrameLayerBuilder.cpp:3144:23
 1:50.93 GECKO(518898)     #12 0x7f44c66db420 in mozilla::PaintedLayerDataNode::Finish(bool) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/painting/FrameLayerBuilder.cpp:3113:3

   ....  see attached log for full trace ...

```


The attached log was produced by the following patch.
```
diff --git a/layout/xul/tree/nsTreeBodyFrame.cpp b/layout/xul/tree/nsTreeBodyFrame.cpp
--- a/layout/xul/tree/nsTreeBodyFrame.cpp
+++ b/layout/xul/tree/nsTreeBodyFrame.cpp
@@ -1593,20 +1593,36 @@ nsresult nsTreeBodyFrame::RowCountChange
   if (aCount == 0 || !mView) return NS_OK;  // Nothing to do.
 
 #ifdef ACCESSIBILITY
   if (PresShell::IsAccessibilityActive()) {
     FireRowCountChangedEvent(aIndex, aCount);
   }
 #endif  // #ifdef ACCESSIBILITY
 
-  // Adjust our selection.
-  nsCOMPtr<nsITreeSelection> sel;
-  mView->GetSelection(getter_AddRefs(sel));
-  if (sel) sel->AdjustSelection(aIndex, aCount);
+  // Validate pointer. Bug 1677194
+  AutoWeakFrame weakFrame(this);
+  nsCOMPtr<nsITreeView> view = mView;
+
+  // Adjust our selection. Note the use of |view| instead of |mView| 
+  nsCOMPtr<nsITreeSelection> sel = nullptr; 
+#if DEBUG
+  printf("RowCountChanged(): view = %p, sel = %p, this = %p\n", (void *) view, (void *) sel, (void *) this);
+  fflush(stdout);
+#endif
+  nsresult rc = view->GetSelection(getter_AddRefs(sel));
+  // Check
+  NS_ENSURE_STATE(weakFrame.IsAlive());
+
+#if DEBUG
+  printf("RowCountChanged(): view = %p, sel = %p, this = %p\n", (void *) view, (void *) sel, (void *) this);
+  fflush(stdout);
+#endif
+
+  if (NS_SUCCEEDED(rc) && sel) sel->AdjustSelection(aIndex, aCount);
 
   if (mUpdateBatchNest) return NS_OK;
 
   mRowCount += aCount;
 #ifdef DEBUG
   int32_t rowCount = mRowCount;
   mView->GetRowCount(&rowCount);
   NS_ASSERTION(

```

Back to Bug 1677194 Comment 7