Closed Bug 377149 Opened 13 years ago Closed 13 years ago

stop storing mask and clip-path frames

Categories

(Core :: SVG, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical?] post 1.8-branch)

Attachments

(3 files, 5 obsolete files)

Storing frames is bad as we get into problems tracking their destruction.

Possibilities:

a) store the element in the property. Would this work properly with reference counting?

b) create another class to store the properties as per marker and filter frames. And store an nsrefptr to the element in that. Downside is that you store the pointer to this class which has a pointer to the element so you use twice the memory. Having said that you only use it if you have one so perhaps this is the way to go.
Attached image mask crash
Whiteboard: [sg:critical?] 1.9-only
You may need to force a refresh e.g. by covering the circle with another application and uncovering it again after you have clicked on click me in order to bang.
having thought about it some more, without a mutation listener you don't know when the elements will disappear so neither a) nor b) are any good so we could do:

c) don't use properties at all and just look up the frames every time you want to use them.

d) put in mutation listener classes for mask and clip-path as per filters and markers
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [sg:critical?] 1.9-only → [sg:critical?] post 1.8-branch
Attached patch patch (obsolete) — Splinter Review
also makes mask and clip paths somewhat live.
Assignee: general → longsonr
Status: NEW → ASSIGNED
Attachment #261364 - Flags: review?(tor)
Attached image testcase
shape should disappear when clicked on.
This seems to have considerable overlap with bug 374598 (and to a lesser extent bug 377015).
Attached patch address review comment (obsolete) — Splinter Review
Update to tip. 

bug 377015 has landed and I've gone for the destructor approach throughout so that filter, clipPath and mask mutation observers are all now like nsSVGMarkerProperty.
Attachment #261364 - Attachment is obsolete: true
Attachment #261812 - Flags: review?(tor)
Attachment #261364 - Flags: review?(tor)
Attached patch diff -w (obsolete) — Splinter Review
If this is OK I'll make bug 374598 invalid.
In nsSVGMaskFrame::ComputeMaskAlpha and nsSVGMaskFrame::GetCanvasTM, you can look at the mask element's fields directly instead of calling mask->Get...Units().

This changes our behavior of a display:none filter - before we would ignore the filter, but now it looks like we won't draw anything.  While neither is per spec, the former behavior seems closer to what we should be doing.
Attached patch address review comments (obsolete) — Splinter Review
(In reply to comment #10)
> In nsSVGMaskFrame::ComputeMaskAlpha and nsSVGMaskFrame::GetCanvasTM, you can
> look at the mask element's fields directly instead of calling
> mask->Get...Units().

Done.

> 
> This changes our behavior of a display:none filter - before we would ignore the
> filter, but now it looks like we won't draw anything.  While neither is per
> spec, the former behavior seems closer to what we should be doing.
> 

Doesn't the code already do that via bug 35775 :-) Anyway I think this new patch addresses that issue.
Attachment #261812 - Attachment is obsolete: true
Attachment #261813 - Attachment is obsolete: true
Attachment #262149 - Flags: review?(tor)
Attachment #261812 - Flags: review?(tor)
(In reply to comment #11)
> > This changes our behavior of a display:none filter - before we would ignore the
> > filter, but now it looks like we won't draw anything.  While neither is per
> > spec, the former behavior seems closer to what we should be doing.
> 
> Doesn't the code already do that via bug 35775 :-) Anyway I think this new
> patch addresses that issue.

I'm not sure what bug you intended to point to, but the existing code won't mark the frame as filtered because it won't be able to find a filter frame.  Your patch would mark it filtered because it only looks for the element.


(In reply to comment #12)
> I'm not sure what bug you intended to point to, but the existing code won't
> mark the frame as filtered because it won't be able to find a filter frame. 
> Your patch would mark it filtered because it only looks for the element.
> 

I'm failing to see something then because this looks to me like you get the frame marked both with/without this patch. There may be a problem with clipping though as you need a frame to determine whether to clip trivial or not.

   if (style->mFilter && !(aFrame->GetStateBits() & NS_STATE_SVG_FILTERED)) {
     nsIContent *filter = NS_GetSVGFilterElement(style->mFilter,
                                                 aFrame->GetContent());
-    if (filter) {
-      nsSVGFilterProperty *property = new nsSVGFilterProperty(filter, aFrame);
-      if (!property) {
-        NS_ERROR("Could not create filter property");
-        return;
-      }
-      NS_ADDREF(property); // addref to allow QI - FilterPropertyDtor releases
-      aFrame->SetProperty(nsGkAtoms::filter,
-                          NS_STATIC_CAST(nsISupports*, property),
-                          nsPropertyTable::SupportsDtorFunc);
-      aFrame->AddStateBits(NS_STATE_SVG_FILTERED);
+    if (filter && !new nsSVGFilterProperty(filter, aFrame)) {
+      NS_ERROR("Could not create filter property");
+      return;
     }
   }
 
The current patch does fix this though I think, the frame is marked filtered whenever there is an element and then PaintChildWithEffect draws it unfiltered when it can't find a frame.
A concern about this patch is that you only add the clip bits if the frame is around, so StyleEffects might end up leaving a property around for a lot longer than is necessary.  Two possibilities for fixing this are to unconditionally try removing the property, or switch to a single clip state bit and adjust nsSVGUtils to query the complexity when needed.
Attached patch address review comment (obsolete) — Splinter Review
Went for a single clip bit.
Attachment #262149 - Attachment is obsolete: true
Attachment #262489 - Flags: review?(tor)
Attachment #262149 - Flags: review?(tor)
Attachment #262489 - Flags: review?(tor) → review+
Attachment #262489 - Flags: superreview?(roc)
Comment on attachment 262489 [details] [diff] [review]
address review comment

+  nsWeakPtr mObserved;

Say mObservedContent, to make it clear what's being observed?

-  if (state & NS_STATE_SVG_CLIPPED_MASK) {
+  if (state & NS_STATE_SVG_CLIPPED)
     aFrame->DeleteProperty(nsGkAtoms::clipPath);
-  }

Don't trim off these braces.
Attachment #262489 - Flags: superreview?(roc) → superreview+
Attachment #262489 - Attachment is obsolete: true
checked in.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Duplicate of this bug: 374598
Flags: wanted1.8.1.x-
Flags: wanted1.8.0.x-
Group: security
You need to log in before you can comment on or make changes to this bug.