Bugzilla

Aha, so the hashtable assertion is happening because we're clearing the hashtable **while we're iterating over it** (and in particular, we're probably even removing the entry that we are currently iterating across).

Stack level 61 is FlushPendingScrollAnchorAdjustments() which has:
```
void nsIPresShell::FlushPendingScrollAnchorAdjustments() {
  for (auto iter = mPendingScrollAnchorAdjustment.Iter(); !iter.Done();
       iter.Next()) {
    nsIScrollableFrame* scroll = iter.Get()->GetKey();
    scroll->GetAnchor()->ApplyAdjustments();
```

And while we're inside of that ApplyAdjustments call, we end up trying to modify the "mPendingScrollAnchorAdjustment" table that our for-loop is currently iterating over. This happens ~55 stack levels deeper, in nsIPresShell::NotifyDestroyingFrame, in this call:
```
    nsIScrollableFrame* scrollableFrame = do_QueryFrame(aFrame);
    if (scrollableFrame) {
      mPendingScrollAnchorSelection.RemoveEntry(scrollableFrame);
      mPendingScrollAnchorAdjustment.RemoveEntry(scrollableFrame);
    }
```

(In my backtrace, the nsIScrollableFrame pointers are actually different between these two stack-levels -- i.e. the scrollable frame that we're currently destroying isn't the same one that we're currently doing scroll-anchor adjustments for.  But it's still bad, because mutation-during-iteration is unsafe for many data structures, presumably including this hash table.)

One part of the solution might be for FlushPendingScrollAnchorAdjustments() to iterate over a **copy** of the hashtable.  

Tangent: initially, I was going to say that we even transfer the contents of the hashtable to a local variable and iterate over that -- but that's actually not a good idea, because if we have two adjustments queued up and the first adjustment kills off the second adjustment's scrollframe, then we'll end up still calling GetAnchor()->ApplyAdjustments on that second (destroyed!) scrollframe, because NotifyDestroyingFrame won't have been able to remove it.

So: perhaps we want to:
 - **copy** mPendingScrollAnchorAdjustment and iterate over the copy.
 - At the start of each loop, check whether the scrollframe that we're currently iterating over still exists in mPendingScrollAnchorAdjustment (i.e. see if NotifyDestroyingFrame removed it) -- and if it doesn't, then skip this loop iteration.
 - And on top of that, I think we also need the change that you've already got in your patch (making ApplyAdjustments() friendly to things having been cleared).


Or alternately/instead, maybe we need to decouple the scrollframe adjustment and the (currently-synchronous) caret layout flush?  I don't know enough about the caret & scroll-observers to know how feasible that is though.

Aha, so the hashtable assertion is happening because we're modifying the hashtable **while we're iterating over it**.

Stack level 61 is FlushPendingScrollAnchorAdjustments() which has:
```
void nsIPresShell::FlushPendingScrollAnchorAdjustments() {
  for (auto iter = mPendingScrollAnchorAdjustment.Iter(); !iter.Done();
       iter.Next()) {
    nsIScrollableFrame* scroll = iter.Get()->GetKey();
    scroll->GetAnchor()->ApplyAdjustments();
```

And while we're inside of that ApplyAdjustments call, we end up trying to modify the "mPendingScrollAnchorAdjustment" table that our for-loop is currently iterating over. This happens ~55 stack levels deeper, in nsIPresShell::NotifyDestroyingFrame, in this call:
```
    nsIScrollableFrame* scrollableFrame = do_QueryFrame(aFrame);
    if (scrollableFrame) {
      mPendingScrollAnchorSelection.RemoveEntry(scrollableFrame);
      mPendingScrollAnchorAdjustment.RemoveEntry(scrollableFrame);
    }
```

(In my backtrace, the nsIScrollableFrame pointers are actually different between these two stack-levels -- i.e. the scrollable frame that we're currently destroying isn't the same one that we're currently doing scroll-anchor adjustments for.  But it's still bad, because mutation-during-iteration is unsafe for many data structures, presumably including this hash table.)

One part of the solution might be for FlushPendingScrollAnchorAdjustments() to iterate over a **copy** of the hashtable.  

Tangent: initially, I was going to say that we even transfer the contents of the hashtable to a local variable and iterate over that -- but that's actually not a good idea, because if we have two adjustments queued up and the first adjustment kills off the second adjustment's scrollframe, then we'll end up still calling GetAnchor()->ApplyAdjustments on that second (destroyed!) scrollframe, because NotifyDestroyingFrame won't have been able to remove it.

So: perhaps we want to:
 - **copy** mPendingScrollAnchorAdjustment and iterate over the copy.
 - At the start of each loop, check whether the scrollframe that we're currently iterating over still exists in mPendingScrollAnchorAdjustment (i.e. see if NotifyDestroyingFrame removed it) -- and if it doesn't, then skip this loop iteration.
 - And on top of that, I think we also need the change that you've already got in your patch (making ApplyAdjustments() friendly to things having been cleared).


Or alternately/instead, maybe we need to decouple the scrollframe adjustment and the (currently-synchronous) caret layout flush?  I don't know enough about the caret & scroll-observers to know how feasible that is though.

Aha, so the hashtable assertion is happening because we're modifying the hashtable **while we're iterating over it**.

Stack level 61 is FlushPendingScrollAnchorAdjustments() which has:
```
void nsIPresShell::FlushPendingScrollAnchorAdjustments() {
  for (auto iter = mPendingScrollAnchorAdjustment.Iter(); !iter.Done();
       iter.Next()) {
    nsIScrollableFrame* scroll = iter.Get()->GetKey();
    scroll->GetAnchor()->ApplyAdjustments();
```

And while we're inside of that ApplyAdjustments call, we end up trying to modify the "mPendingScrollAnchorAdjustment" table that our for-loop is currently iterating over. This happens ~55 stack levels deeper, in nsIPresShell::NotifyDestroyingFrame, in this call:
```
    nsIScrollableFrame* scrollableFrame = do_QueryFrame(aFrame);
    if (scrollableFrame) {
      mPendingScrollAnchorSelection.RemoveEntry(scrollableFrame);
      mPendingScrollAnchorAdjustment.RemoveEntry(scrollableFrame);
    }
```

(In my backtrace, the nsIScrollableFrame pointers are actually different between these two stack-levels -- i.e. the scrollable frame that we're currently destroying isn't the same one that we're currently doing scroll-anchor adjustments for.  But it's still bad, because mutation-during-iteration is unsafe for many data structures, presumably including this hash table.)

One part of the solution might be for FlushPendingScrollAnchorAdjustments() to iterate over a **copy** of the hashtable.  

Tangent: initially, I was going to say that we even transfer the contents of the hashtable to a local variable and iterate over that -- but that's actually not a good idea, because if we have two adjustments queued up and the first adjustment kills off the second adjustment's scrollframe, then we'll end up still calling GetAnchor()->ApplyAdjustments on that second (destroyed!) scrollframe, because NotifyDestroyingFrame won't have been able to remove it.

~So: perhaps we want to:~ [EDIT: never mind, we don't want this. We really need something that prevents layout flushing entirely, per emilio's comment 9]
 - **copy** mPendingScrollAnchorAdjustment and iterate over the copy.
 - At the start of each loop, check whether the scrollframe that we're currently iterating over still exists in mPendingScrollAnchorAdjustment (i.e. see if NotifyDestroyingFrame removed it) -- and if it doesn't, then skip this loop iteration.
 - And on top of that, I think we also need the change that you've already got in your patch (making ApplyAdjustments() friendly to things having been cleared).


Or alternately/instead, maybe we need to decouple the scrollframe adjustment and the (currently-synchronous) caret layout flush?  I don't know enough about the caret & scroll-observers to know how feasible that is though.