"ASSERTION: wrong frame type" with svg requiredExtensions, unsuspendRedrawAll

RESOLVED FIXED in mozilla7

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: Robert Longson)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla7
x86
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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]
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.
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 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.
Attachment #532408 - Attachment is obsolete: true
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.
Attachment #532413 - Flags: review?(dholbert)
(Assignee)

Comment 5

6 years ago
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.
(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.
(Assignee)

Comment 7

6 years ago
<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.
(Assignee)

Comment 8

6 years ago
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.
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.
(Assignee)

Comment 10

6 years ago
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.
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. :-)
(Assignee)

Comment 12

6 years ago
Created attachment 535948 [details] [diff] [review]
simpler patch

Seems simplest to just remove the assertions as they aren't valid any more.
Attachment #535948 - Flags: review?(dholbert)
Attachment #535948 - Flags: feedback?(cam)
Comment on attachment 535948 [details] [diff] [review]
simpler patch

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

The approach seems fine to me.
Attachment #535948 - Flags: feedback?(cam) → feedback+
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
Attachment #535948 - Flags: review?(dholbert) → review+
(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)
(Assignee)

Updated

6 years ago
Attachment #532413 - Attachment is obsolete: true
Attachment #532413 - Flags: review?(dholbert)
(Assignee)

Comment 16

6 years ago
Created attachment 536358 [details] [diff] [review]
address review comment and add tests
(Assignee)

Updated

6 years ago
Attachment #536358 - Attachment description: z → address review comment and add tests
Attachment #536358 - Attachment is patch: true
(Assignee)

Updated

6 years ago
Attachment #535948 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/40cbdfc8dba1
Assignee: nobody → longsonr
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Updated

6 years ago
Blocks: 327706
(Assignee)

Updated

6 years ago
Blocks: 385552
(Assignee)

Updated

6 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.