Bug 10735 – Clicking in SVG results causes WebKit to consume 100% CPU for several minutes

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Mark Rowe (bdash)]

Steps to reproduce:

1) Load URL
2) Wait for it to finish downloading + rendering
3) Click in the middle of the image
4) Note that Safari has become unresponsive
5) Wait patiently for it to finish processing

[Mark Rowe (bdash)]

I have tested with r16068 and r16225 and see the problem in both.

Most of the time is spent in one call to VisiblePosition::canonicalPosition(), and under that in one call to each of Position::upstream(), Position::downstream(), nextCandidate() and previousCandidate(). The position to canonicalize is sitting in a very wide tree of unrendered content. The above 4 calls are O(n^2) since they essentially loop using Position::next() which in turn half the time uses Node::nodeIndex() which is O(n).

[Eric Seidel (no email)]

I'm raising this to a P1 since this is effectively a crash.

It looks like SVG (possibly because of how many nodes it generates as opposed to HTML) just sent the position code off the deep end.  It's also possible that this is caused by bounding-box calculation issues in SVG.

[Eric Seidel (no email)]

Created attachment 10523 [details]
Sample during the hang.

[Dave Hyatt]

Maybe try subclassing positionForCoordinates on all SVG objects to return an empty VisiblePosition... with no element at all.... e.g., VisiblePosition(0, 0, DOWNSTREAM);

[Stephanie Lewis]

In Radar
<rdar://problem/4946826>

Created attachment 13216 [details]
[WIP] use an iterator that doesn't maintain child indices

Passes all the editing tests but I need to review it once more to verify that it is correct, look for more places that can benefit from using the new PositionIterator, possibly split it out into its own files, and figure out if PositionIterator needs to be optimized for the common(?) case of not iterating at all (the optimization would be to cache the initial Position in the PositionIterator, not initialize its other member variables until the first increment/decrement, and if those were never called, do the conversion to Position by returning the cached one).

Some bits of correctness I'm concerned about include editingIgnoresContent()-positive nodes, <br>s and negative offsets.

Created attachment 13228 [details]
Use an iterator that doesn't maintain child indices

No layout test regressions.

Note that there used to be a PositionIterator class in WebCore, but it was quite different than the proposed one.

Created attachment 13237 [details]
Use an iterator that doesn't maintain child indices

See comment #8. Fixed typos and removed one redundant statement.

[Darin Adler]

Comment on attachment 13237 [details]
Use an iterator that doesn't maintain child indices

+friend class PositionIterator;

Normaly we'd indent this kind of friend declaration.

Could we make the helper functions public instead of private? Then PositionIterator would not need to be a friend.

Why does Position need to be a friend of PositionIterator?

r=me, anyway

Comment on attachment 13237 [details]
Use an iterator that doesn't maintain child indices

Going to make a new patch

Created attachment 13243 [details]
Use an iterator that doesn't maintain child indices

(In reply to comment #10)
> Could we make the helper functions public instead of private? Then
> PositionIterator would not need to be a friend.

Done.

> Why does Position need to be a friend of PositionIterator?

The Position(const PositionIterator&) constructor accesses private members. I left it this way in this patch because it's the lesser evil of the alternatives I could think of.

Also fixed the indentation of the friend declaration and moved another star.

[Darin Adler]

Comment on attachment 13243 [details]
Use an iterator that doesn't maintain child indices

r=me

I'd like to see makefiles updated for platforms other than Mac.

Created attachment 13258 [details]
Use an iterator that doesn't maintain child indices

(In reply to comment #13)
> I'd like to see makefiles updated for platforms other than Mac.

Done.

[Sam Weinig]

Comment on attachment 13258 [details]
Use an iterator that doesn't maintain child indices

Really only reviewing the updates to the makefiles/build systems for non-mac platforms.  r=me!

[Sam Weinig]

Landed in r19734.