371563 - Crash [@ nsCSSFrameConstructor::FindFrameWithContent] with SVG marker, float, overflow: scroll

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (usually crashes within a few seconds)

Security-sensitive because this bug causes Firefox (debug) to dereference 0xdddddddd.

Comment 1

[Daniel Veditz [:dveditz]]

This testcase does not crash Firefox 2.0.0.2

roc: if you are not the right assignee please find a home for this critical bug.

Comment 2

[Robert Longson [:longsonr]]

I'll take a stab at replacing the marker observer code with a mutation observer implementation (c.f. bug 359516 which did it for filters) to see if that fixes it.

Comment 3

[Jesse Ruderman]

Won't that cause problems for DOM mutations that occur during load?  Or did bug 90983 only apply to DOM events, not nsIMutationObserver?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes. nsIMutationObserver events are always sent out. Layout is built almost entierly on top of those notifications.

Comment 5

[Robert Longson [:longsonr]]

Attachment: patch

Comment 6

[tor]

Won't this have the problems that Jesse has pointed out with similar implementations, namely hanging references if an ancestor of the <marker> is deleted?

Comment 7

[Robert Longson [:longsonr]]

(In reply to comment #6)
> Won't this have the problems that Jesse has pointed out with similar
> implementations, namely hanging references if an ancestor of the <marker> is
> deleted?
> 

Wouldn't NodeWillBeDestroyed be called, causing everything to be destroyed?

If not, your comment applies to filters too doesn't it? In any case I'm not sure what I, if anything I should do?

Comment 8

[tor]

(In reply to comment #7)
> (In reply to comment #6)
> > Won't this have the problems that Jesse has pointed out with similar
> > implementations, namely hanging references if an ancestor of the <marker> is
> > deleted?
> 
> Wouldn't NodeWillBeDestroyed be called, causing everything to be destroyed?

That only gets called when the object is really destroyed (last reference is dropped).  Due to lazy JS garbage collection this could happen some time after it is removed from the tree.  

> If not, your comment applies to filters too doesn't it? In any case I'm not
> sure what I, if anything I should do?

Yes, it is a problem with filters - see bug 369438.

Comment 9

[Robert Longson [:longsonr]]

(In reply to comment #8)
> Yes, it is a problem with filters - see bug 369438.
> 

Could you cc me to that one please? I can't access it.

Comment 10

[tor]

(In reply to comment #9)
> (In reply to comment #8)
> > Yes, it is a problem with filters - see bug 369438.
> 
> Could you cc me to that one please? I can't access it.

CC'd.

Comment 11

[Robert Longson [:longsonr]]

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

I'll see how bug 369438 pans out then. Should this one block on that?

Comment 12

[Robert Longson [:longsonr]]

Attachment: update for changed API

Comment 13

[tor]

If you're removing the observers on the nsSVGMarkerFrame, you can remove that frame's inheritance of nsSVGValue.

Comment 14

[Robert Longson [:longsonr]]

Attachment: address review comment

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 258406 [details] [diff] [review]
address review comment

Request review from myself to remind me to see if I'm happy to review this instead of roc.

Comment 16

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

Comment on attachment 258406 [details] [diff] [review]
address review comment

+  void GetMarkerFromStyle(MarkerPosition markerPosition,

aMarkerPosition

+  if (markerFrame &&
+      mMarkerStartFrame != markerFrame &&
+      mMarkerMidFrame != markerFrame &&
+      mMarkerEndFrame != markerFrame) {

This is a bit weird. Why not just compare to *aMarkerFrame here? Then we know when we're done that *aMarkerFrame is set to markerFrame. As it is, it could keep its old value if the new frame is used for one of the other markers.

With that, sr=roc

Comment 17

[Robert Longson [:longsonr]]

Attachment: version for check in

with sr comments addressed.

Comment 18

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

That's not what I meant!

The test should be
  if (markerFrame && *aMarkerFrame != markerFrame)
... right?

Comment 19

[Robert Longson [:longsonr]]

(In reply to comment #18)
> That's not what I meant!
> 
> The test should be
>   if (markerFrame && *aMarkerFrame != markerFrame)
> ... right?
> 

I don't think so *aMarkerFrame  would always be null here.

If you have <rect  marker-mid="url(#marker1)" marker-end="url(#marker1)" ... /> then marker-mid and marker-end will get the same pointer from the NS_GetSVGMarkerFrame call. I want to avoid having two observers in that case so I to check whether the marker frame is the same as any of the existing marker frames. If it is then the existing marker would have an observer so I don't need to create another one.

You were correct that the original code did have a bug that if there was an existing identical marker frame the new code would not return the correct pointer.

Comment 20

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

I don't think you should do this optimization. It's too tricky and I don't see any reason why it would be important. The mObserverMarker* values should always match the mMarker*Frame values.

In particular, I think your latest code has a bug where e.g. if I set the start marker and then the end marker to the same element E, and then set the start marker to another element, we're not observing E anymore even though we should be because it's still the end marker.

Comment 21

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

Comment on attachment 258981 [details] [diff] [review]
version for check in

Please don't check this in until things are sorted out.

Comment 22

[Robert Longson [:longsonr]]

Attachment: address further sr comments

(In reply to comment #20)
> I don't think you should do this optimization. It's too tricky and I don't see
> any reason why it would be important. The mObserverMarker* values should always
> match the mMarker*Frame values.

True, it is not required as AddMutationObserver already checks for duplicates. FWIW the code it replaces did not do that at the time I wrote it. I've removed the check and also updated RemoveMutationObserver so that it nulls out the observed marker once the observer has been removed.

> 
> In particular, I think your latest code has a bug where e.g. if I set the start
> marker and then the end marker to the same element E, and then set the start
> marker to another element, we're not observing E anymore even though we should
> be because it's still the end marker.
> 

That's not a problem. Marker changes are style updates so if you change one nsSVGPathGeometryFrame::DidSetStyleContext is called and all the marker observers are discarded so you start again each time.

Comment 23

[Jonathan Watt [:jwatt]]

Oops, I'm too slow. BTW roc, requesting review from myself wasn't a hint that you're to slow. On the contrary, you do an awesome job of getting through your review queue considering the number of review requests you get. Rather my review request was an acknowledgment that your do get a lot of requests, and we should try and take some of that load where possible.

Comment 24

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

Comment on attachment 258986 [details] [diff] [review]
address further sr comments

+    nsCOMPtr<nsIContent> markerContent = (*aMarkerFrame)->GetContent();

I think this could just be an nsIContent*.

jwatt: thanks, but this was an sr request so I need to do it, right?

Comment 25

[Jonathan Watt [:jwatt]]

tor's review could have counted as sr and mine as peer review - anyway, that's academic now.

Comment 26

[Robert Longson [:longsonr]]

Attachment: version for check in

(In reply to comment #24)
> (From update of attachment 258986 [details] [diff] [review])
> +    nsCOMPtr<nsIContent> markerContent = (*aMarkerFrame)->GetContent();
> 
> I think this could just be an nsIContent*.
> 

Changed as suggested.

I also reverted RemoveMutationObserver not to null its argument as it is only called from the destructor so the pointers are going away anyway. RemoveMutationObserver is now back to what it was when tor reviewed it.

Comment 27

[Jonathan Watt [:jwatt]]

Without having looked at the code, that sounds like a bad idea to me. Functions should not make assumptions about who is calling them, and besides, that could change in future. I'm also against being "clever"/"optimizing" where pointers to freed memory is concerned. Somewhere, sometime it'll go wrong and the consequences are potentially way, way too serious to make it worth the negligible optimizations. IMHO pointers to freed memory should always be nulled out. Always.

Comment 28

[Jonathan Watt [:jwatt]]

When I say "freed memory" I mean that in the most general sense. I.e. I'm including out param pointers that aren't assigned anything valid.

Comment 29

[Robert Longson [:longsonr]]

We're not freeing any memory in RemoveMutationObserver just using the pointer to remove the mutation observer. The pointer is still valid.

Comment 30

[Jonathan Watt [:jwatt]]

Okay, I was just going on what your comment sounded like. Anyway, you guys know what you're doing.

Comment 31

[Robert Longson [:longsonr]]

checked in.

Comment 32

[Jesse Ruderman]

Crashtest checked in.