876831 - "ASSERTION: destroy called on frame while scripts not blocked" with bidi

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

With:
  user_pref("svg.text.css-frames.enabled", true);

###!!! ASSERTION: destroy called on frame while scripts not blocked: '!nsContentUtils::IsSafeToRunScript()', file layout/generic/nsFrame.cpp, line 598

###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file layout/base/nsCSSFrameConstructor.cpp, line 1490

Comment 1

[Jesse Ruderman]

Attachment: stacks

Comment 2

[Cameron McCormack (:heycam)]

I think we have a problem with how reflow of the anonymous block from of nsSVGTextFrame2 is handled.  I think this bug and some previous ones show that it's not safe to reflow the anonymous block frame child with a call to kid->Reflow() when we're under a DOM call like getComputedTextLength or document.caretPositionFromPoint like in the test case here.  But we do need to call kid->Reflow() directly if we're reflowing under ReflowSVG.

So I think we need to handle these two cases separately.  I tried making nsSVGTextFrame2::UpdateGlyphPositioning do this:

  if (nsSVGUtils::OuterSVGIsCallingReflowSVG(this)) {
    nsPresContext::InterruptPreventer noInterrupts(PresContext());
    DoReflow(aForceGlobalTransform);
  } else {
    PresContext()->GetPresShell()->FlushPendingNotifications(Flush_Layout);
  }

so that we only ever call DoReflow underneath ReflowSVG.  This doesn't work for non-display children of <pattern>, etc., since the FlushPendingNotification call will never end up reflowing down to the non-display nsSVGTextFrame2.

But it does work if I use the patch from bug 873806 underneath it, and then make the ReflowSVGText function in nsSVGContainerFrame.cpp also call ReflowSVG on the non-display nsSVGTextFrame2.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: patch

Do you think this is the right direction Jonathan?  We could have the ReflowSVGText function from bug 873806 call a new function on nsSVGTextFrame2 rather than ReflowSVG, if you'd prefer ReflowSVG to continue not doing anything for non-display frames.

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Cameron McCormack (:heycam) from comment #2)
> I think this bug and some previous ones show
> that it's not safe to reflow the anonymous block frame child with a call to
> kid->Reflow() when we're under a DOM call like getComputedTextLength or
> document.caretPositionFromPoint like in the test case here.

Doesn't it just show that we need to add an nsAutoScriptBlocker to the stack in nsSVGTextFrame2::UpdateGlyphPositioning?

Comment 5

[Cameron McCormack (:heycam)]

I did try that, and that leads to an assertion saying that frames can only be destroyed inside a document update (i.e. surrounded with BeginUpdate / EndUpdate calls to the document).  And in bug 843917 comment 4 it sounded like doing a document update isn't the right thing.  This is what lead me to believe that reflowing outside of a normal document reflow isn't the right thing to do.

Comment 6

[Jonathan Watt [:jwatt]]

So I think you should probably get review from dbaron or bz on this one. I'd have thought that this is kinda "normal" since if script changes layout and then gets something that depends on layout, we need to flush. I'm not familiar with how we do that "correctly" though.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: patch (v2)

Here's an updated patch on top of the new bug 873806 patch.

To summarise, this patch ensures that we never synchronously call Reflow() on our anonymous block child when we are not under a normal document reflow.  This avoids these problems of assertions due to scripts not being safe to run and not being within a document update.  So there are now two cases when we get in to UpdateGlyphPositioning:

  1. we are under ReflowSVG, in which case we want do want to just call Reflow
     on our anonymous block child, since we're in reflow, and
  2. we are not under ReflowSVG, in which case we call FlushPendingNotifications
     to flush layout.

Since reflowing of non-display nsSVGTextFrame2 frames is now handled through ReflowSVG on an ancestor searching through its non-display children to find the nsSVGTextFrame2s to do the actual reflow, we need to make sure that style updates of nsSVGTextFrame2s cause ReflowSVG to be called -- and thus cause an actual reflow to occur.  So I've added a separate ScheduleReflowSVGNonDisplayText function that will set frame state bits appropriately and call FrameNeedsReflow, similarly to how nsSVGUtils::ScheduleReflowSVG does it, and I call that from the newly added DidSetStyleContext that handles the bug 873806 comment 13 case you brought up.  We can't call nsSVGTextFrame2::ReflowSVGNonDisplayText from in there since we're not in reflow; so instead we schedule it.

So r?jwatt for the patch as a whole, and r?bz to make sure that it makes sense to handle reflow of my anonymous block child of nsSVGTextFrame2 by calling FlushPendingNotifications rather than calling Reflow immediately.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 755825 [details] [diff] [review]
patch (v2)

>+             "do not call ReflowSVGNonDisplayText when the outer SVG frame is "

Wrong function name?

>+      if ((f->GetStateBits() &
>+           (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN))) {

  if (NS_SUBTREE_DIRTY(f)) {

>+      PresContext()->GetPresShell()->FlushPendingNotifications(Flush_Layout);

This is fine, but it can run script and destroy frames and such.  So after this call you need to not touch any members on this frame or any other frames.

r=me with that last bit audited/fixed

Comment 9

[Boris Zbarsky [:bzbarsky]]

Also, I assume there's a reason why we need a sync reflow there, not an async one, right?

Comment 10

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> >+      PresContext()->GetPresShell()->FlushPendingNotifications(Flush_Layout);
> 
> This is fine, but it can run script and destroy frames and such.  So after
> this call you need to not touch any members on this frame or any other
> frames.

Hmm.  I didn't realise that.  We do a bunch of work after this call -- basically this UpdateGlyphMetrics call is at the start of all of the SVG DOM methods, PaintSVG, hit testing methods, and anything that requires the child frames to be up to date.  I see some other frame classes that do things like keep a hold of themselves with an nsWeakFrame to see if they got destroyed during that call.

The tricky case that this FlushPendingNotifications is meant to handle is when you're in script, and you do a DOM modification on a child of a <text>, and then you immediately call in to some DOM method that makes its way to the nsSVGTextFrame2 that requires the child frames to have been reflowed.  Can the nsSVGTextFrame2 really disappear at this point though?  This is sounding difficult to ensure.

Comment 11

[Cameron McCormack (:heycam)]

To be safe, though, we could do the nsWeakFrame thing and just return an error from UpdateGlyphPositioning, and all the callers of UpdateGlyphPositioning can just return early if that call failed because the nsSVGTextFrame2 disappeared.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Can the nsSVGTextFrame2 really disappear at this point though? 

Yes.  Most simply, if there is a pending but not processed yet style change to display:none that gets flushed out when you do the flush.

Comment 11 sounds like the most reasonable plan here.

Comment 13

[Boris Zbarsky [:bzbarsky]]

However, note that unless the callers are ancestors it's possible for them to disappear without the nsSVGTextFrame2 disappearing, right?

Comment 14

[Cameron McCormack (:heycam)]

Yes.  I'll make sure that any callers of UpdateGlyphPositioning don't assume that the anonymous block kid or any other descendant still exists after the call.

Comment 15

[Boris Zbarsky [:bzbarsky]]

They need to not assume that any frame that was not an ancestor of the nsWeakFrame exists, assuming the nsWeakFrame did not die.  If the nsWeakFrame died, you don't know anything at all about what objects might or might not exist (e.g. there might have been a window.close() followed by some event loop spinning under there).

Comment 16

[Cameron McCormack (:heycam)]

Attachment: patch (v2.1)

Right.  So this is what I have now.  Does that look sane?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 756318 [details] [diff] [review]
patch (v2.1)

So what happens if we take the early return in FindCloserFrameForSelection, say?  What does the caller end up doing then?

Also, see the other parts of comment 8...

Comment 18

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky (:bz) from comment #17)
> So what happens if we take the early return in FindCloserFrameForSelection,
> say?  What does the caller end up doing then?

Ah yes, that's a bad case.  It calls GetNextSibling() on it.  So that one should be eWithoutReflow.  IIRC FindCloserFrameForSelection is only used while interactively selecting things, so it should be OK to just avoid reflowing at that point.  I don't think we could actually have out of date frames at that point.

I'll do a bit more checking of callers-of-callers of UpdateGlyphPositioning.

> Also, see the other parts of comment 8...

Forgot those.

Comment 19

[Cameron McCormack (:heycam)]

I am getting progressively more concerned about this approach.

Boris pointed out to me that we really should have reflowed the anonymous block child before we get into the methods on nsSVGTextFrame2.  For example, for calling getComputedTextLength(), the method on SVGTextContentElement flushes layout before calling in to nsSVGTextFrame2.  So I need to look into why we're still reflowing in UpdateGlyphPositioning.

If we do keep the reflow in UpdateGlyphPositioning, then we're in danger of the script blocker that we need to surround it with causing the whole frame tree to be destroyed.  So even if we tried to limit the reflow to the anonymous block frame, we're at risk of this.

The other difficult case is PaintSVG.  We have many callers of this -- not just nsDisplaySVGText -- and it would not be great to have to make sure each of them could survive the frame disappearing.  Having to reflow in paint does not seem right anyway.  We currently rely on PaintSVG doing the reflow of the child for non-display text, e.g. when it's in a <mask>, etc.  We should be able to ensure that the frame gets reflowed during the regular document reflow though.

The work in bug 873806 should have made non-display nsSVGTextFrame2s get reflowed, so I'll try to determine why that's not the case.

Comment 20

[Cameron McCormack (:heycam)]

Attachment: patch (v3)

Well.  It turns out that with the aforementioned work in bug 873806, we don't actually have a problem where the anonymous block frame needs to be reflowed by the time we're in PaintSVG, or one of the SVG DOM methods, etc.  Hooray!  This makes me a lot happier.

UpdateGlyphPositioning has changed to only do the mPositions recomputation.  For the two cases where we want to reflow the anonymous block frame just before this -- in nsSVGTextFrame2::ReflowSVG (for displayed <text>), and in ReflowSVGNonDisplayText, which is called from under nsSVGDisplayContainerFrame::ReflowSVG (for undisplayed <text>) -- we call MaybeReflowAnonymousBlockChild which does that work.  This is always going to be under a reflow, and we assert that now.

r?bz for the much saner reflow logistics, and r?jwatt for good measure.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 756426 [details] [diff] [review]
patch (v3)

Also, see the other parts of comment 8...  again.

>   if (mState & (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN)) {

NS_SUBTREE_DIRTY(this) here too.

r=me.  I like this a lot more.  ;)

Comment 22

[Cameron McCormack (:heycam)]

Attachment: patch (v3.1)

Updated slightly to rebase on top of bug 873806, but not materially different.

Comment 23

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f19c792705d

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f19c792705d