Last Comment Bug 657077 - "ASSERTION: wrong frame type" with svg requiredExtensions, unsuspendRedrawAll
: "ASSERTION: wrong frame type" with svg requiredExtensions, unsuspendRedrawAll
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla7
Assigned To: Robert Longson
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 326633 327706 385552
  Show dependency treegraph
 
Reported: 2011-05-13 17:10 PDT by Jesse Ruderman
Modified: 2011-06-01 11:40 PDT (History)
4 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (243 bytes, image/svg+xml)
2011-05-13 17:10 PDT, Jesse Ruderman
no flags Details
Make SVGSVGElement::(un)suspendRedraw use the outer svg element's frame in case this one's doesn't exist. (2.98 KB, patch)
2011-05-13 20:42 PDT, Cameron McCormack (:heycam)
no flags Details | Diff | Splinter Review
Make SVGSVGElement::(un)suspendRedraw use the outer svg element's frame in case this one's doesn't exist (v2) (7.01 KB, patch)
2011-05-13 21:40 PDT, Cameron McCormack (:heycam)
no flags Details | Diff | Splinter Review
simpler patch (4.12 KB, patch)
2011-05-29 10:58 PDT, Robert Longson
dholbert: review+
cam: feedback+
Details | Diff | Splinter Review
address review comment and add tests (5.40 KB, patch)
2011-05-31 11:33 PDT, Robert Longson
no flags Details | Diff | Splinter Review

Description Jesse Ruderman 2011-05-13 17:10:46 PDT
Created attachment 532388 [details]
testcase

###!!! ASSERTION: wrong frame type: 'svgframe', file /builds/slave/cen-osx64-dbg/build/content/svg/content/src/nsSVGSVGElement.cpp, line 446

nsSVGSVGElement::UnsuspendRedrawAll [content/svg/content/src/nsSVGSVGElement.cpp:447]
NS_InvokeByIndex_P [xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:195]
Comment 1 Cameron McCormack (:heycam) 2011-05-13 17:14:17 PDT
The inner svg element won't have a frame, since it failed the conditional attribute test.  nsSVGSVGElement::UnsuspendRedrawAll probably should get the frame for the outer svg element, since we always create a frame for it.
Comment 2 Cameron McCormack (:heycam) 2011-05-13 20:42:48 PDT
Created attachment 532408 [details] [diff] [review]
Make SVGSVGElement::(un)suspendRedraw use the outer svg element's frame in case this one's doesn't exist.
Comment 3 Cameron McCormack (:heycam) 2011-05-13 20:45:46 PDT
Comment on attachment 532408 [details] [diff] [review]
Make SVGSVGElement::(un)suspendRedraw use the outer svg element's frame in case this one's doesn't exist.

Scratch that, I'll do it a little differently.
Comment 4 Cameron McCormack (:heycam) 2011-05-13 21:40:18 PDT
Created attachment 532413 [details] [diff] [review]
Make SVGSVGElement::(un)suspendRedraw use the outer svg element's frame in case this one's doesn't exist (v2)

I've removed the assertion about the (un)suspend methods being called when
there is no frame, since that can validly happen when the <svg> element is
display:none.

I've repurposed the assertion about the frame being of the right type since
inner <svg> elements get nsSVGContainerElement frames when they have failing
conditional attributes.  So the assertion now checks the inner <svg> frame
either implements nsISVGSVGFrame or is an nsSVGContainerFrame.

Outer <svg> element frames must either be nsSVGOuterSVGFrames (or null when
display:none), so I've added an assertion for that.

In cases where we validly don't have an nsISVGSVGFrame to call, we now
return NS_ERROR_FAILURE, which I think is more accurate than pretending
the call succeeded.
Comment 5 Robert Longson 2011-05-14 01:41:09 PDT
So given the testcase structure a suspendRedraw on the element id="a" would change the outer svg element's mRedrawSuspendCount. If we then did removeAttribute("requiredExtensions") followed by an unsuspendRedraw we'd be unsuspending this frame rather than the outer svg frame we suspended.

On top of that if the id="a" frame had a rect sibling, we'd stop drawing that if we suspended the outer svg frame's redraw.

On the whole I think it's best to ignore the suspend/unsuspend if the frame has a NS_STATE_SVG_NONDISPLAY_CHILD state bit set. Same as if you couldn't find the frame.
Comment 6 Cameron McCormack (:heycam) 2011-05-14 01:48:12 PDT
(In reply to comment #5)
> So given the testcase structure a suspendRedraw on the element id="a" would
> change the outer svg element's mRedrawSuspendCount. If we then did
> removeAttribute("requiredExtensions") followed by an unsuspendRedraw we'd be
> unsuspending this frame rather than the outer svg frame we suspended.

That's a good point.

> On top of that if the id="a" frame had a rect sibling, we'd stop drawing
> that if we suspended the outer svg frame's redraw.

Is that not the desired behaviour?  I always thought of suspendRedraw, regardless of which SVGSVGElement you call it on, would suspend redrawing of the entire SVG fragment. 

> On the whole I think it's best to ignore the suspend/unsuspend if the frame
> has a NS_STATE_SVG_NONDISPLAY_CHILD state bit set. Same as if you couldn't
> find the frame.

If redraw suspension is meant to be per SVG fragment (and not just the subtree rooted on the particular <svg> you call it on), then I say we allow this to work.
Comment 7 Robert Longson 2011-05-14 02:14:56 PDT
<svg>
   <svg id="a" requiredExtensions="x"/>
   <rect/>
</svg>

Doing a suspendRedraw on the "a" element should not stop the rect drawing should it? If so we should always go to the outer svg frame/element and suspend that, if not we should never go to the outer svg frame/element.
Comment 8 Robert Longson 2011-05-14 02:20:31 PDT
The only hint given is that http://www.w3.org/TR/SVG/struct.html#__svg__SVGSVGElement__forceRedraw says that it forces the redraw of the viewport and each svg element forms its own viewport so that suggests that it's just the inner svg that's suspended. The suspendRedraw text does not however say what gets suspended.
Comment 9 Daniel Holbert [:dholbert] 2011-05-17 14:38:46 PDT
Sorry for the delay on this.

Patch looks good to me, but I think Comment 7 - Comment 8 brings up a good point about whether the patch does the right thing.

Given that forceRedraw's effects are internal to the fragment that it's called on (paraphrasing comment 8), I'd intuitively expect suspendRedraw to also be per-fragment.

I honestly don't think I've looked at our (Un)SuspendRedraw code before today :) so longsonr is probably a better person to r+ the final patch here.

But I think my leaning would be to go along with longsonr's hunch in comment 7 / comment 8, unless there's justification in the spec or www-svg (or from an interop standpoint) for the "GetThisOrOuterSVGFrame" behavior.
Comment 10 Robert Longson 2011-05-18 01:22:09 PDT
How about...

a) remove the mRedrawSuspendCount variable from the outer svg frame and implement an IsRedrawSuspended in the nsSVGSVGElement. 

b) Methods on the nsSVGOuterSVGFrame should call their equivalent methods on the nsSVGSVGElement e.g. SuspendRedraw would call nsSVGSVGElement::SuspendRedraw and the code currently in nsSVGOuterSVGFrame would move to nsSVGSVGElement::SuspendRedraw

Getting the primary frame would not be necessary any more.
Comment 11 Cameron McCormack (:heycam) 2011-05-24 22:19:39 PDT
Sounds OK to me.  I still think redraw suspension is intended to be a global thing, but I admit the spec doesn't really say anything to support that conclusion. :-)
Comment 12 Robert Longson 2011-05-29 10:58:08 PDT
Created attachment 535948 [details] [diff] [review]
simpler patch

Seems simplest to just remove the assertions as they aren't valid any more.
Comment 13 Cameron McCormack (:heycam) 2011-05-29 22:20:55 PDT
Comment on attachment 535948 [details] [diff] [review]
simpler patch

Review of attachment 535948 [details] [diff] [review]:
-----------------------------------------------------------------

The approach seems fine to me.
Comment 14 Daniel Holbert [:dholbert] 2011-05-31 05:09:29 PDT
Comment on attachment 535948 [details] [diff] [review]
simpler patch

>diff --git a/content/svg/content/src/nsSVGSVGElement.cpp b/content/svg/content/src/nsSVGSVGElement.cpp
>   if (frame) {
>     nsISVGSVGFrame* svgframe = do_QueryFrame(frame);
>-    NS_ASSERTION(svgframe, "wrong frame type");
>+    // might not be true if we've failed conditional processing
>     if (svgframe) {

Looks good, but "might not be true" bothers me a little readability-wise, since I think of the if-check as "check whether this frame is non-null" rather than "check whether this frame is 'true'".

Maybe replace "might not be true" with "might fail this check"?

r=dholbert with that
Comment 15 Daniel Holbert [:dholbert] 2011-05-31 05:11:34 PDT
(In reply to comment #14)
> Looks good, but "might not be true" bothers me a little readability-wise
(in part because |svgFrame| will never be _equal_ to true or PR_TRUE, which IIRC are both exactly 1)
Comment 16 Robert Longson 2011-05-31 11:33:26 PDT
Created attachment 536358 [details] [diff] [review]
address review comment and add tests
Comment 17 Dão Gottwald [:dao] 2011-06-01 01:46:46 PDT
http://hg.mozilla.org/mozilla-central/rev/40cbdfc8dba1

Note You need to log in before you can comment on or make changes to this bug.