Bug 1993379 Comment 3 Edit History

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

As discussed separately, it seems like the issue here is that   ~TextRenderedRunIterator() assumes `mFrameIterator.Root()` is non-null; but we don't have that guarantee.

In this case, Tyson let me know that the testcase hits bug 1467887's assertion in debug builds, which means `FrameIfAnonymousChildReflowed(` will be returning nullptr in opt builds; and so we'll be passing nullptr to initialize mFrameIterator (as its first arg) here:
https://searchfox.org/firefox-main/rev/dc1c78e9c37aba6ed05a4ec47c4bfcb16f57b51d/layout/svg/SVGTextFrame.cpp#1756-1759
```cpp
explicit TextRenderedRunIterator(SVGTextFrame* aSVGTextFrame,
                                 RenderedRunFilter aFilter = eAllFrames,
                                 const nsIFrame* aSubtree = nullptr)
    : mFrameIterator(FrameIfAnonymousChildReflowed(aSVGTextFrame), aSubtree),
```

It handles a null mRootFrame nicely internally, e.g. here:
https://searchfox.org/firefox-main/rev/dc1c78e9c37aba6ed05a4ec47c4bfcb16f57b51d/layout/svg/SVGTextFrame.cpp#1568-1571
```cpp
void Init() {
  if (!mRootFrame) {
    return;
  }
```

So it needs to handle null mRootFrame nicely at destruction-time too.

I propose:
(1) We null-check Root() before using it, in the TextRenderedRunIterator destructor.
(2) Perhaps we we rename Root() and Current() and Next() to have a "Get" prefix, since they return pointers which need to be null-checked (cannot be assumed to be non-null), per our naming convention.
As discussed separately, it seems like the issue here is that   ~TextRenderedRunIterator() assumes `mFrameIterator.Root()` is non-null; but we don't have that guarantee.

In this case, Tyson let me know that the testcase hits bug 1467887's assertion in debug builds, which means `FrameIfAnonymousChildReflowed(` will be returning nullptr in opt builds; and so we'll be passing nullptr to initialize mFrameIterator (as its first arg) here:
https://searchfox.org/firefox-main/rev/dc1c78e9c37aba6ed05a4ec47c4bfcb16f57b51d/layout/svg/SVGTextFrame.cpp#1756-1759
```cpp
explicit TextRenderedRunIterator(SVGTextFrame* aSVGTextFrame,
                                 RenderedRunFilter aFilter = eAllFrames,
                                 const nsIFrame* aSubtree = nullptr)
    : mFrameIterator(FrameIfAnonymousChildReflowed(aSVGTextFrame), aSubtree),
```

It handles a null mRootFrame nicely internally, e.g. here:
https://searchfox.org/firefox-main/rev/dc1c78e9c37aba6ed05a4ec47c4bfcb16f57b51d/layout/svg/SVGTextFrame.cpp#1568-1571
```cpp
void Init() {
  if (!mRootFrame) {
    return;
  }
```

So it needs to handle null mRootFrame nicely at destruction-time too.

I propose:
(1) We null-check Root() before using it, in the TextRenderedRunIterator destructor.
(2) Perhaps we we rename TextFrameIterator::Root() and Current() and Next() to have a "Get" prefix, since they return pointers which need to be null-checked (cannot be assumed to be non-null), per our naming convention.

Back to Bug 1993379 Comment 3