843917 - "ASSERTION: destroy called on frame while scripts not blocked"

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 610

It might be important that the second character is both astral and missing, and I might not have picked the best codepoint to guarantee that it's missing.

Comment 1

[Jesse Ruderman]

Attachment: stack (gdb)

Comment 2

[Cameron McCormack (:heycam)]

Attachment: patch

roc, I tried using a script blocker in nsSVGTextFrame2::DoReflow, but that then just resulted in an assertion saying that frame destruction should only happen while in an update.  So I added this.  Does this make sense at all?  I don't really know what mozAutoDocUpdate is for, apart from noting that an update is in progress, or whether it really makes sense to use it underneath the nsDocument::EndUpdate call.  It does prevent any assertions with this test, and doesn't cause problems with any of my other tests, though...

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That sounds like a question for Boris.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Which assert are you hitting for the "frame destruction" bit?

In any case, the way presshell does this is as follows:

1)  An nsScriptBlocker on the stack.
2)  WillDoReflow and DidDoReflow calls (with the latter happening after the
    scriptblocker has gone out of scope, which seems slightly fishy but may actually
    be required).

I really doubt you want to be doing a document-level update here...

One other note: at the point when the scriptblocker goes out of scope script will be able to run.  So make sure to put that high enough up your callstack.

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 729405 [details] [diff] [review]
patch

Thanks for the info Boris.

As it turns out, others changes I've made have avoided this assertion being triggered, so no need for the patch on this bug.

Comment 6

[Cameron McCormack (:heycam)]

(Not true; I didn't realise I had the patch applied locally.)

So here is what happens.  I hit that assertion at the top of nsFrame::DestroyFrom nsFrame.cpp:597 with this on the stack:

http://www.pastebin.mozilla.org/2297671

where the AppendChild at the bottom is called from script.  The GlyphMetricsUpdater gets run at this point because:

* when the AppendChild DOM mutation happens, we call nsSVGTextFrame2::NotifyGlyphMetricsChange
* this adds a GlyphMetricsUpdater script runner, since we don't want to do its work immediately after the DOM mutation but before the frames under the nsSVGTextFrame2 have been reflowed
* the reason we want to delay the GlyphMetricsUpdater is that it will call nsSVGUtils::InvalidateBounds on the nsSVGTextFrame2, but when a <text> is filtered, the frames beneath it must be up to date so that we can report a bounding box to the filter so our filter region can be computed to invalidate

So this setup seems to work, and inside nsSVGTextFrame2::GetBBoxContribution we'll reflow its anonymous block child, since we need this up to date to compute our bbox contribution.  The assertion is hit because we don't have a script blocker set at the time the reflow of these frames happens.

Now, if I wrap the guts of nsSVGTextFrame2::DoReflow with a nsContentUtils::{Add,Remove}ScriptBlocker pair, this avoids the assertion but lands me in this:

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

shortly after, which is at the top of nsCSSFrameConstructor::NotifyDestroyingFrame.


As it happens, using the document-level update got me in trouble with a different bug (it triggered a DOM generation increment while I'm in the middle of PaintInactiveLayer, which caused another assertion about not modifying the DOM in the middle of a paint).


So I'm confused.  Is it fundamentally wrong to be reflowing my child anonymous block frame at this point?  Is it wrong for nsSVGUtils::InvalidateBounds to rely on the anonymous block frame being reflowed?  Should I instead be dispatching a runnable to do everything from mozilla::GlyphMetricsUpdater::Run downwards even later than when the script blocker is removed at the end of the AppendChild call?  And if so, is there a way I could do this so that it does this before the next paint?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Is it fundamentally wrong to be reflowing my child anonymous block frame at this point? 

At first glance, probably not.

> Is it wrong for nsSVGUtils::InvalidateBounds to rely on the anonymous block frame being
> reflowed?

Imo, somewhat, mostly as a matter of performance: forcing sync reflow is not great.

> Should I instead be dispatching a runnable

No.

All you have to do is do the dance that reflow expects to happen; see comment 4.

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky (:bz) from comment #7)
> All you have to do is do the dance that reflow expects to happen; see
> comment 4.

I tried with an nsAutoScriptBlocker:

  http://pastebin.mozilla.org/2297768

and even with it going out of script before the DidReflow, I get the "Should be in an update while destroying frames" assertion.  Did I miss some steps in the dance?

Comment 9

[Jonathan Watt [:jwatt]]

Backing up a bit, why do you need the nsSVGUtils::InvalidateBounds call here? You shouldn't need it any more? I'd suggest that you remove it, and replace it with:

  nsSVGEffects::InvalidateRenderingObservers(aFrame);

If you have invalidation issues as a result of doing that then you can also try adding:

  if (!(aFrame->GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD)) {
    aFrame->InvalidateFrameSubtree();
  }

but if you have to add that, we'll need to figure out why it's necessary.

Anyway, ping me on IRC and I can help work through this.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Did I miss some steps in the dance?

Yes.  Calling PresShell::WillReflow/DidReflow, which is what starts and stops updates and does some other bookkeeping.

Comment 11

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky (:bz) from comment #10)
> Yes.  Calling PresShell::WillReflow/DidReflow, which is what starts and
> stops updates and does some other bookkeeping.

Those methods don't seem to exist; I was calling methods named like these on the frame itself, though.

But...

(In reply to Jonathan Watt [:jwatt] from comment #9)
> Backing up a bit, why do you need the nsSVGUtils::InvalidateBounds call
> here? You shouldn't need it any more? I'd suggest that you remove it, and
> replace it with:
> 
>   nsSVGEffects::InvalidateRenderingObservers(aFrame);

This works for me.

Jonathan, I guess I don't understand why InvalidateBounds() is not required at all.  Or is it just that it's not required when it's a non-display child?  Because GlyphMetricsUpdater::Run will be called even for changes to normally displayed <text> elements.  Is it because these changes will cause a reflow of the child frame, and will get invalidated due to that?  (Which is different from other SVG frame classes, where we really do need to call InvalidateBounds() in response to for example attribute changes?)

Comment 12

[Jonathan Watt [:jwatt]]

DLBI should take care of invalidating the areas that have changed nowadays, as long as it's informed that it should create a new display list to compare with the previous one (something that it will be informed about under the FrameNeedsReflow() call). This is the case for displayed content (non-display content doesn't need to invalidate, it just needs to tell any rendering observers that they should invalidate). The fact that other SVG frames need to call InvalidateBounds() is a bug that I'm hoping to get a chance to work on very soon.

Comment 13

[Cameron McCormack (:heycam)]

Attachment: patch (v2)

Thanks, that makes sense.  I don't have any problems with normal text invalidation without the InvalidateBounds() call.

Also, running the SVG text reftests locally for me is now a lot faster with this patch.  Before it must have been doing too much invalidation work.

Comment 14

[Jonathan Watt [:jwatt]]

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


(In reply to Cameron McCormack (:heycam) from comment #13)
> Also, running the SVG text reftests locally for me is now a lot faster with
> this patch.  Before it must have been doing too much invalidation work.

Yeah, in cases where there aren't too many elements to display, DLBI is much faster.

Comment 15

[Cameron McCormack (:heycam)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1077b98a75f1

Comment 16

[Robert Longson [:longsonr]]

Can you check in the testcase as a crashtest please?

Comment 17

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53c2e7b9753b

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Those methods don't seem to exist;

Er, I mean WillDoReflow/DidDoReflow.  :(  I got it right in comment 4!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1077b98a75f1
https://hg.mozilla.org/mozilla-central/rev/53c2e7b9753b