WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
103914
Add support for tracking hit test rectangles to enable fast event rejection in the compositor
https://bugs.webkit.org/show_bug.cgi?id=103914
Summary
Add support for tracking hit test rectangles to enable fast event rejection i...
Levi Weintraub
Reported
2012-12-03 11:45:51 PST
When the WebKit thread is in heavy use, it can be faster to perform a first-pass hit test in the compositor before handing the event to WebKit. This change adds support for tracking the rects associated with an event target with an event handler, and begins doing so for Touch Events on Chromium.
Attachments
Patch
(47.31 KB, patch)
2012-12-03 12:08 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Patch
(48.36 KB, patch)
2012-12-03 18:10 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Patch
(48.35 KB, patch)
2012-12-04 10:33 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Patch
(48.33 KB, patch)
2012-12-04 14:44 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Patch
(48.36 KB, patch)
2012-12-04 17:22 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Patch
(50.82 KB, patch)
2012-12-07 12:35 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Patch
(50.93 KB, patch)
2012-12-07 14:09 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Patch
(50.91 KB, patch)
2012-12-07 14:41 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Patch
(50.94 KB, patch)
2012-12-10 10:02 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Simplified solution
(33.71 KB, patch)
2012-12-11 12:16 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Patch
(32.82 KB, patch)
2012-12-11 14:19 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Patch for landing
(28.66 KB, patch)
2012-12-12 16:15 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Patch for landing
(36.46 KB, patch)
2012-12-12 16:20 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Patch for landing
(36.44 KB, patch)
2012-12-12 16:45 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Patch
(40.85 KB, patch)
2012-12-14 17:30 PST
,
Levi Weintraub
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Patch
(39.54 KB, patch)
2012-12-16 19:08 PST
,
Levi Weintraub
no flags
Details
Formatted Diff
Diff
Show Obsolete
(15)
View All
Add attachment
proposed patch, testcase, etc.
Levi Weintraub
Comment 1
2012-12-03 12:08:13 PST
Created
attachment 177303
[details]
Patch
Eric Seidel (no email)
Comment 2
2012-12-03 12:22:55 PST
Do we know how iOS survives w/o this? :)
Build Bot
Comment 3
2012-12-03 12:38:21 PST
Comment on
attachment 177303
[details]
Patch
Attachment 177303
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/15104615
James Robinson
Comment 4
2012-12-03 12:47:38 PST
Comment on
attachment 177303
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=177303&action=review
> Source/WebCore/rendering/RenderInline.cpp:1437 > + Vector<IntRect> result;
here and elsewhere - can you use a Region for this? It would accumulate overlapping / enclosed rects in a slightly nicer way
Simon Fraser (smfr)
Comment 5
2012-12-03 12:59:45 PST
Comment on
attachment 177303
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=177303&action=review
> Source/WebCore/rendering/RenderBlock.cpp:6931 > +void RenderBlock::addLayerHitTestRects(LayerHitTestRects& layerRects, const RenderLayer* currentCompositedLayer, const LayoutPoint& layerOffset) const
I don't understand why you're accumulating these rects relative to some RenderLayer. Shouldn't they all just be root-relative? Also the layerOffset stuff won't work with software-rendered transforms.
> Source/WebCore/rendering/RenderBlock.cpp:6960 > + if (width() && height()) > + result.append(pixelSnappedIntRect(adjustedLayerOffset, size())); > + > + if (hasHorizontalLayoutOverflow() || hasVerticalLayoutOverflow()) { > + for (RootInlineBox* curr = firstRootBox(); curr; curr = curr->nextRootBox()) { > + LayoutUnit top = max<LayoutUnit>(curr->lineTop(), curr->top()); > + LayoutUnit bottom = min<LayoutUnit>(curr->lineBottom(), curr->top() + curr->height()); > + LayoutRect rect(adjustedLayerOffset.x() + curr->x(), adjustedLayerOffset.y() + top, curr->width(), bottom - top); > + if (!rect.isEmpty()) > + result.append(pixelSnappedIntRect(rect)); > + } > + } > + > + for (RenderObject* curr = firstChild(); curr; curr = curr->nextSibling()) { > + if (!curr->isText() && !curr->isListMarker() && curr->isBox()) { > + RenderBox* box = toRenderBox(curr); > + box->addLayerHitTestRects(layerRects, currentCompositedLayer, adjustedLayerOffset + box->locationOffset()); > + } > + }
Rather than have all this code here, can we make a generic RenderObject method for computing the hit-test region for an arbitrary element?
> Source/WebCore/rendering/RenderObject.cpp:629 > + // Don't apply transforms as we should never pass through one. > + mapLocalToContainer((*enclosingCompositedLayer)->renderer(), transformState, ApplyContainerFlip | SnapOffsetForTransforms);
Sure you will; 2D transforms don't always make compositing layers.
Levi Weintraub
Comment 6
2012-12-03 13:23:43 PST
Comment on
attachment 177303
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=177303&action=review
>> Source/WebCore/rendering/RenderBlock.cpp:6931 >> +void RenderBlock::addLayerHitTestRects(LayerHitTestRects& layerRects, const RenderLayer* currentCompositedLayer, const LayoutPoint& layerOffset) const > > I don't understand why you're accumulating these rects relative to some RenderLayer. Shouldn't they all just be root-relative? > > Also the layerOffset stuff won't work with software-rendered transforms.
Accumulating them relative to their enclosing composited layer allows us to hit test those layers in the compositor properly when 3d transforms are involved, ie as opposed to taking an enclosing bounding box in the root's space.
>> Source/WebCore/rendering/RenderBlock.cpp:6960 >> + } > > Rather than have all this code here, can we make a generic RenderObject method for computing the hit-test region for an arbitrary element?
With the differing implementations for boxes, blocks, and inlines, it seems uglier to me to probe for types, but it's certainly do-able.
>> Source/WebCore/rendering/RenderInline.cpp:1437 >> + Vector<IntRect> result; > > here and elsewhere - can you use a Region for this? It would accumulate overlapping / enclosed rects in a slightly nicer way
Sure.
>> Source/WebCore/rendering/RenderObject.cpp:629 >> + mapLocalToContainer((*enclosingCompositedLayer)->renderer(), transformState, ApplyContainerFlip | SnapOffsetForTransforms); > > Sure you will; 2D transforms don't always make compositing layers.
Good catch, thanks!
Eric Seidel (no email)
Comment 7
2012-12-03 13:27:01 PST
Comment on
attachment 177303
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=177303&action=review
>>> Source/WebCore/rendering/RenderInline.cpp:1437 >>> + Vector<IntRect> result; >> >> here and elsewhere - can you use a Region for this? It would accumulate overlapping / enclosed rects in a slightly nicer way > > Sure.
Very very slowly. :) Unioning N rects is N^2 with Region. :( See
bug 98800
Simon Fraser (smfr)
Comment 8
2012-12-03 13:33:02 PST
(In reply to
comment #6
)
> (From update of
attachment 177303
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=177303&action=review
> > >> Source/WebCore/rendering/RenderBlock.cpp:6931 > >> +void RenderBlock::addLayerHitTestRects(LayerHitTestRects& layerRects, const RenderLayer* currentCompositedLayer, const LayoutPoint& layerOffset) const > > > > I don't understand why you're accumulating these rects relative to some RenderLayer. Shouldn't they all just be root-relative? > > > > Also the layerOffset stuff won't work with software-rendered transforms. > > Accumulating them relative to their enclosing composited layer allows us to hit test those layers in the compositor properly when 3d transforms are involved, ie as opposed to taking an enclosing bounding box in the root's space.
I see. That does seem rather platform-specific, though. You could track root-relative quads. On iOS, we track them all root-relative (as bounding boxes). It would be nice to be able to share code here.
> >> Source/WebCore/rendering/RenderBlock.cpp:6960 > >> + } > > > > Rather than have all this code here, can we make a generic RenderObject method for computing the hit-test region for an arbitrary element? > > With the differing implementations for boxes, blocks, and inlines, it seems uglier to me to probe for types, but it's certainly do-able.
Use virtual methods? It would be akin to absoluteQuads().
Simon Fraser (smfr)
Comment 9
2012-12-03 13:33:56 PST
Also, do you track rects per-frame, or just on the root including all subframes?
Levi Weintraub
Comment 10
2012-12-03 14:09:15 PST
Comment on
attachment 177303
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=177303&action=review
>>>> Source/WebCore/rendering/RenderBlock.cpp:6931 >>>> +void RenderBlock::addLayerHitTestRects(LayerHitTestRects& layerRects, const RenderLayer* currentCompositedLayer, const LayoutPoint& layerOffset) const >>> >>> I don't understand why you're accumulating these rects relative to some RenderLayer. Shouldn't they all just be root-relative? >>> >>> Also the layerOffset stuff won't work with software-rendered transforms. >> >> Accumulating them relative to their enclosing composited layer allows us to hit test those layers in the compositor properly when 3d transforms are involved, ie as opposed to taking an enclosing bounding box in the root's space. > > I see. That does seem rather platform-specific, though. You could track root-relative quads. > > On iOS, we track them all root-relative (as bounding boxes). It would be nice to be able to share code here.
Root-relative boxes or quads would be possible, it just requires a lot more layer walking. I'd agree it'd be nice to share code. Is the code used to generate these bounding boxes on iOS available to share? Obviously, the standard bounding box code doesn't actually mirror hit testing, but if there's something that does this already I'd love to re-use it.
>>>> Source/WebCore/rendering/RenderBlock.cpp:6960 >>>> + } >>> >>> Rather than have all this code here, can we make a generic RenderObject method for computing the hit-test region for an arbitrary element? >> >> With the differing implementations for boxes, blocks, and inlines, it seems uglier to me to probe for types, but it's certainly do-able. > > Use virtual methods? It would be akin to absoluteQuads().
If our goal is to generate all the absolute bounding boxes or quads, we can simplify the virtual implementations.
>>>> Source/WebCore/rendering/RenderInline.cpp:1437 >>>> + Vector<IntRect> result; >>> >>> here and elsewhere - can you use a Region for this? It would accumulate overlapping / enclosed rects in a slightly nicer way >> >> Sure. > > Very very slowly. :) Unioning N rects is N^2 with Region. :( See
bug 98800
I originally implemented it with Regions, but after looking at the accumulation code there I changed my path.
James Robinson
Comment 11
2012-12-03 16:43:02 PST
Doing everything viewport-relative would meet our short-term needs, but doing hit testing on child layers is better long term (since it's more precise) and something I would expect the ScrollingTree logic to be able to handle eventually even on iOS.
Simon Fraser (smfr)
Comment 12
2012-12-03 17:12:01 PST
The scrolling tree isn't about layers; it's about "islands of scrollability".
James Robinson
Comment 13
2012-12-03 17:28:38 PST
OK(In reply to
comment #12
)
> The scrolling tree isn't about layers; it's about "islands of scrollability".
OK, well I think the points still stands - you'd want to be able to hit test on each "island" separately since they will move around relative to each other. I don't really know what the point of trying to abstract here is since the only useful islands of scrollability are ones you can move (i.e. layers)
Levi Weintraub
Comment 14
2012-12-03 18:10:34 PST
Created
attachment 177386
[details]
Patch
Levi Weintraub
Comment 15
2012-12-03 18:14:09 PST
(In reply to
comment #14
)
> Created an attachment (id=177386) [details] > Patch
This patch switches to using repaint containers, but it doesn't drastically re-architect the method for accumulating the rects. I found trying to do the tree walk in a separate non-virtual function to be quite ugly.
Levi Weintraub
Comment 16
2012-12-03 18:16:22 PST
(In reply to
comment #4
)
> (From update of
attachment 177303
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=177303&action=review
> > > Source/WebCore/rendering/RenderInline.cpp:1437 > > + Vector<IntRect> result; > > here and elsewhere - can you use a Region for this? It would accumulate overlapping / enclosed rects in a slightly nicer way
If this gets traction, I'll file another bug to track switching to Regions when performance allows.
Levi Weintraub
Comment 17
2012-12-03 18:24:25 PST
(In reply to
comment #16
)
> (In reply to
comment #4
) > > (From update of
attachment 177303
[details]
[details]) > > View in context:
https://bugs.webkit.org/attachment.cgi?id=177303&action=review
> > > > > Source/WebCore/rendering/RenderInline.cpp:1437 > > > + Vector<IntRect> result; > > > > here and elsewhere - can you use a Region for this? It would accumulate overlapping / enclosed rects in a slightly nicer way > > If this gets traction, I'll file another bug to track switching to Regions when performance allows.
(For the record, the bug tracking the N^2 issue is
https://bugs.webkit.org/show_bug.cgi?id=100814
)
Build Bot
Comment 18
2012-12-03 18:40:18 PST
Comment on
attachment 177386
[details]
Patch
Attachment 177386
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/15101793
Levi Weintraub
Comment 19
2012-12-04 10:33:58 PST
Created
attachment 177507
[details]
Patch
Build Bot
Comment 20
2012-12-04 11:22:59 PST
Comment on
attachment 177507
[details]
Patch
Attachment 177507
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/15154039
Levi Weintraub
Comment 21
2012-12-04 14:44:20 PST
Created
attachment 177569
[details]
Patch
Build Bot
Comment 22
2012-12-04 16:00:46 PST
Comment on
attachment 177569
[details]
Patch
Attachment 177569
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/15152139
Levi Weintraub
Comment 23
2012-12-04 17:22:24 PST
Created
attachment 177615
[details]
Patch
Levi Weintraub
Comment 24
2012-12-05 09:51:48 PST
(In reply to
comment #23
)
> Created an attachment (id=177615) [details] > Patch
smfr: see if this addresses your concerns.
Levi Weintraub
Comment 25
2012-12-05 14:15:15 PST
(In reply to
comment #24
)
> (In reply to
comment #23
) > > Created an attachment (id=177615) [details] [details] > > Patch > > smfr: see if this addresses your concerns.
Ping?
James Robinson
Comment 26
2012-12-06 15:50:09 PST
Comment on
attachment 177615
[details]
Patch R=me
Simon Fraser (smfr)
Comment 27
2012-12-06 15:59:17 PST
Comment on
attachment 177615
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=177615&action=review
> Source/WebCore/rendering/RenderBox.cpp:483 > +void RenderBox::addRepaintContainerHitTestRects(RepaintContainerHitTestRects& repaintContainerRects, const RenderLayerModelObject* currentRepaintContainer, const LayoutPoint& repaintOffset) const
What does "current" add to repaintContainer?
> Source/WebCore/rendering/RenderBox.cpp:485 > + LayoutPoint adjustedRepaintOffset = repaintOffset;
It's odd that this is called a repaintOffset when it has nothing to do with (re)painting. It should just be offsetFromContainer.
> Source/WebCore/rendering/RenderBox.cpp:493 > + if (width() && height()) { > + IntRect result(pixelSnappedIntRect(adjustedRepaintOffset, size()));
I'd prefer this to be a call to something like borderBoxRect(). Just using size() doesn't indicate that you've thought about which rect makes sense for hit testing.
> Source/WebCore/rendering/RenderObject.cpp:622 > + // Self-painting layers subtract the renderer's location. Otherwise, we need to map our location to container space.
self-painting layer means something different in RL speak.
> Source/WebCore/rendering/RenderObject.cpp:629 > + TransformState transformState(TransformState::ApplyTransformDirection, FloatPoint()); > + mapLocalToContainer(repaintContainer, transformState, ApplyContainerFlip | UseTransforms | SnapOffsetForTransforms); > + transformState.flatten(); > + offset = LayoutPoint(transformState.lastPlanarPoint());
See comment below.
> Source/WebCore/rendering/RenderObject.h:965 > + void repaintContainerAndOffset(const RenderLayerModelObject*&, LayoutPoint&) const;
r- for this method. This encourages incorrect usage, where this is a software-rendered transform between you and your repaint container. Just offsetting a rect by that offset will do the wrong thing there.
Levi Weintraub
Comment 28
2012-12-06 17:55:27 PST
Comment on
attachment 177615
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=177615&action=review
>> Source/WebCore/rendering/RenderBox.cpp:483 >> +void RenderBox::addRepaintContainerHitTestRects(RepaintContainerHitTestRects& repaintContainerRects, const RenderLayerModelObject* currentRepaintContainer, const LayoutPoint& repaintOffset) const > > What does "current" add to repaintContainer?
The idea is that it can be updated within the method itself, so what's passed in is not necessarily the repaint container.
>> Source/WebCore/rendering/RenderBox.cpp:485 >> + LayoutPoint adjustedRepaintOffset = repaintOffset; > > It's odd that this is called a repaintOffset when it has nothing to do with (re)painting. It should just be offsetFromContainer.
No problem.
>> Source/WebCore/rendering/RenderBox.cpp:493 >> + IntRect result(pixelSnappedIntRect(adjustedRepaintOffset, size())); > > I'd prefer this to be a call to something like borderBoxRect(). Just using size() doesn't indicate that you've thought about which rect makes sense for hit testing.
Sure.
>> Source/WebCore/rendering/RenderObject.h:965 >> + void repaintContainerAndOffset(const RenderLayerModelObject*&, LayoutPoint&) const; > > r- for this method. This encourages incorrect usage, where this is a software-rendered transform between you and your repaint container. Just offsetting a rect by that offset will do the wrong thing there.
Any recommendations for the name then?
Levi Weintraub
Comment 29
2012-12-07 12:35:14 PST
Created
attachment 178256
[details]
Patch
Build Bot
Comment 30
2012-12-07 13:35:52 PST
Comment on
attachment 178256
[details]
Patch
Attachment 178256
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/15175807
WebKit Review Bot
Comment 31
2012-12-07 14:01:58 PST
Comment on
attachment 178256
[details]
Patch
Attachment 178256
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/15182656
New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Levi Weintraub
Comment 32
2012-12-07 14:09:00 PST
Created
attachment 178272
[details]
Patch
Early Warning System Bot
Comment 33
2012-12-07 14:19:17 PST
Comment on
attachment 178272
[details]
Patch
Attachment 178272
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/15186524
Early Warning System Bot
Comment 34
2012-12-07 14:23:40 PST
Comment on
attachment 178272
[details]
Patch
Attachment 178272
[details]
did not pass qt-wk2-ews (qt): Output:
http://queues.webkit.org/results/15181648
Build Bot
Comment 35
2012-12-07 14:40:36 PST
Comment on
attachment 178272
[details]
Patch
Attachment 178272
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/15212265
Levi Weintraub
Comment 36
2012-12-07 14:41:05 PST
Created
attachment 178281
[details]
Patch
Build Bot
Comment 37
2012-12-08 03:46:09 PST
Comment on
attachment 178281
[details]
Patch
Attachment 178281
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/15219101
Levi Weintraub
Comment 38
2012-12-10 10:02:14 PST
Created
attachment 178580
[details]
Patch
Levi Weintraub
Comment 39
2012-12-11 12:16:04 PST
Created
attachment 178845
[details]
Simplified solution It's important to our schedule to get some flavor of this in soon, so in the interest of moving forward, this is a more simple solution that uses clippedOverflowRectForRepaint, mirroring my understanding of what iOS does.
Simon Fraser (smfr)
Comment 40
2012-12-11 13:50:10 PST
Comment on
attachment 178845
[details]
Simplified solution View in context:
https://bugs.webkit.org/attachment.cgi?id=178845&action=review
> Source/WebCore/ChangeLog:50 > + (WebCore):
Please remove all the redundant lines like this one. Per-function comments are useful to explain particular changes that are not covered in the summary.
> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:192 > + rects.append(enclosingIntRect(renderer->clippedOverflowRectForRepaint(0))); > + if (renderer->isRenderBlock()) { > + const RenderBlock* block = toRenderBlock(renderer); > + for (RenderObject* child = block->firstChild(); child; child = child->nextSibling()) > + accumulateRendererTouchEventTargetRects(rects, child);
This is a tree walk which is O(N^2) on tree depth (since clippedOverflowRectForRepaint(0) walks up the root for each renderer). Making a version of clippedOverflowRectForRepaint() which takes a RenderGeometryMap would alleviate that.
> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:204 > + if (iter->key == document) {
I'd prefer iter->key be pulled out into a local variable that makes it clear what its type is.
> Source/WebCore/page/scrolling/ScrollingCoordinator.h:156 > + void computeAbsoluteTouchEventTargetRects(const Document*, Vector<IntRect>&);
Doesn't this need to be inside a #if ENABLE(TOUCH_EVENT_TRACKING)?
Levi Weintraub
Comment 41
2012-12-11 13:56:49 PST
Comment on
attachment 178845
[details]
Simplified solution View in context:
https://bugs.webkit.org/attachment.cgi?id=178845&action=review
>> Source/WebCore/ChangeLog:50 >> + (WebCore): > > Please remove all the redundant lines like this one. Per-function comments are useful to explain particular changes that are not covered in the summary.
Okay.
>> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:192 >> + accumulateRendererTouchEventTargetRects(rects, child); > > This is a tree walk which is O(N^2) on tree depth (since clippedOverflowRectForRepaint(0) walks up the root for each renderer). Making a version of clippedOverflowRectForRepaint() which takes a RenderGeometryMap would alleviate that.
Agreed. I'm happy to revisit this in a future patch.
>> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:204 >> + if (iter->key == document) { > > I'd prefer iter->key be pulled out into a local variable that makes it clear what its type is.
Good idea.
Levi Weintraub
Comment 42
2012-12-11 14:19:25 PST
Created
attachment 178874
[details]
Patch
Levi Weintraub
Comment 43
2012-12-12 10:26:33 PST
smfr, can I get you to take another look?
Eric Seidel (no email)
Comment 44
2012-12-12 15:36:27 PST
Comment on
attachment 178874
[details]
Patch Thank you for your help Simon.
Levi Weintraub
Comment 45
2012-12-12 15:37:05 PST
(In reply to
comment #44
)
> (From update of
attachment 178874
[details]
) > Thank you for your help Simon.
+1! I'll file a tracking bug for the n^2 issue.
WebKit Review Bot
Comment 46
2012-12-12 15:39:20 PST
Comment on
attachment 178874
[details]
Patch Rejecting
attachment 178874
[details]
from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: atching file Source/WebKit/chromium/src/WebPluginContainerImpl.cpp patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/platform/chromium/fast/events/touch/compositor-touch-hit-rects-expected.txt patching file LayoutTests/platform/chromium/fast/events/touch/compositor-touch-hit-rects.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Simon Fras..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output:
http://queues.webkit.org/results/15304166
Levi Weintraub
Comment 47
2012-12-12 15:42:56 PST
Comment on
attachment 178874
[details]
Patch I'm not sure what broke there. Giving it a kick.
WebKit Review Bot
Comment 48
2012-12-12 15:45:06 PST
Comment on
attachment 178874
[details]
Patch Rejecting
attachment 178874
[details]
from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: atching file Source/WebKit/chromium/src/WebPluginContainerImpl.cpp patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/platform/chromium/fast/events/touch/compositor-touch-hit-rects-expected.txt patching file LayoutTests/platform/chromium/fast/events/touch/compositor-touch-hit-rects.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Simon Fras..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output:
http://queues.webkit.org/results/15272948
Levi Weintraub
Comment 49
2012-12-12 16:15:54 PST
Created
attachment 179143
[details]
Patch for landing
Levi Weintraub
Comment 50
2012-12-12 16:19:09 PST
Comment on
attachment 179143
[details]
Patch for landing Missing the tests...
Levi Weintraub
Comment 51
2012-12-12 16:20:19 PST
Created
attachment 179146
[details]
Patch for landing
WebKit Review Bot
Comment 52
2012-12-12 16:24:01 PST
Attachment 179146
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebCore/page/scrolling/ScrollingCoordinator.h:155: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Levi Weintraub
Comment 53
2012-12-12 16:45:53 PST
Created
attachment 179150
[details]
Patch for landing
WebKit Review Bot
Comment 54
2012-12-12 17:36:05 PST
Comment on
attachment 179150
[details]
Patch for landing Rejecting
attachment 179150
[details]
from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output:
http://queues.webkit.org/results/15312043
Levi Weintraub
Comment 55
2012-12-14 17:30:36 PST
Created
attachment 179565
[details]
Patch
WebKit Review Bot
Comment 56
2012-12-14 22:39:39 PST
Comment on
attachment 179565
[details]
Patch
Attachment 179565
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/15317906
New failing tests: http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html http/tests/inspector/network/har-content.html http/tests/inspector/network/network-disable-cache-xhrs.html http/tests/inspector/extensions-headers.html http/tests/inspector-enabled/injected-script-discard.html http/tests/inspector/console-cross-origin-iframe-logging.html accessibility/contenteditable-table-check-causes-crash.html http/tests/inspector/filesystem/delete-entry.html http/tests/inspector/extensions-useragent.html http/tests/inspector-enabled/dynamic-scripts.html http/tests/inspector/indexeddb/resources-panel.html compositing/visibility/hidden-iframe.html http/tests/inspector/network/network-cachedresources-with-same-urls.html http/tests/inspector/console-cd-completions.html accessibility/notification-listeners.html http/tests/inspector/compiler-script-mapping.html http/tests/cache/cancel-during-failure-crash.html http/tests/inspector/filesystem/request-directory-content.html http/tests/inspector/indexeddb/database-names.html http/tests/inspector/web-socket-frame-error.html accessibility/duplicate-child-nodes.html http/tests/inspector/network/cached-resource-destroyed-moved-to-storage.html http/tests/inspector/console-xhr-logging-async.html http/tests/inspector/network/network-empty-xhr.html http/tests/inspector/network/network-cyrillic-xhr.html http/tests/inspector/resource-har-headers.html http/tests/inspector/network-preflight-options.html http/tests/inspector/inspect-iframe-from-different-domain.html http/tests/inspector/filesystem/request-filesystem-root.html http/tests/inspector-enabled/database-open.html
James Robinson
Comment 57
2012-12-15 16:32:24 PST
Crashes seem related - here's one from the unit tests: [ RUN ] WebPageSerializerTest.MultipleFrames Received signal 11 base::debug::StackTrace::StackTrace() [0x1b993ee] base::debug::(anonymous namespace)::StackDumpSignalHandler() [0x1b994f5] <unknown> [0x7f8e29ac2af0] WebCore::Document::didRemoveEventTargetNode() [0x88efe4] WebCore::DOMWindow::~DOMWindow() [0x12158dc] WebCore::Document::~Document() [0x89e553] WebCore::HTMLDocument::~HTMLDocument() [0xcdeb9d] WebCore::Node::~Node() [0x8d8adb] WebCore::HTMLSpanElement::~HTMLSpanElement() [0x1fe57d7] v8::internal::GlobalHandles::PostGarbageCollectionProcessing() [0xa1457b] v8::internal::Heap::PerformGarbageCollection() [0xa335d3] v8::internal::Heap::CollectGarbage() [0xa33e28] v8::internal::Map::UpdateCodeCache() [0xb0b6c3] v8::internal::StubCache::ComputeKeyedLoadOrStoreElement() [0xbacb66] v8::internal::KeyedIC::ComputeStub() [0xa79369] v8::internal::KeyedStoreIC::Store() [0xa799cd] v8::internal::KeyedStoreIC_Miss() [0xa79eda] <unknown> [0x3bc14b206362]
James Robinson
Comment 58
2012-12-15 19:18:21 PST
Definitely crashing - not every time, but here's one example stack from fast/events/touch/ Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0xbbadbf17 0x031588e7 in WTF::IdentityHashTranslator<WTF::PtrHash<WebCore::Node*> >::equal<WebCore::Node*> (a=@0xbbadbf17, b=@0xbfff90f8) at HashTable.h:300 300 template<typename T> static bool equal(const T& a, const T& b) { return HashFunctions::equal(a, b); } #0 0x031588e7 in WTF::IdentityHashTranslator<WTF::PtrHash<WebCore::Node*> >::equal<WebCore::Node*> (a=@0xbbadbf17, b=@0xbfff90f8) at HashTable.h:300 #1 0x0237537e in WTF::HashTable<WebCore::Node*, WTF::KeyValuePair<WebCore::Node*, unsigned int>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::Node*, unsigned int> >, WTF::PtrHash<WebCore::Node*>, WTF::HashMapValueTraits<WTF::HashTraits<WebCore::Node*>, WTF::HashTraits<unsigned int> >, WTF::HashTraits<WebCore::Node*> >::lookup<WTF::IdentityHashTranslator<WTF::PtrHash<WebCore::Node*> >, WebCore::Node*> (this=0xe236760, key=@0xbfff90f8) at HashTable.h:628 #2 0x023751ec in WTF::HashTable<WebCore::Node*, WTF::KeyValuePair<WebCore::Node*, unsigned int>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::Node*, unsigned int> >, WTF::PtrHash<WebCore::Node*>, WTF::HashMapValueTraits<WTF::HashTraits<WebCore::Node*>, WTF::HashTraits<unsigned int> >, WTF::HashTraits<WebCore::Node*> >::find<WTF::IdentityHashTranslator<WTF::PtrHash<WebCore::Node*> >, WebCore::Node*> (this=0xe236760, key=@0xbfff90f8) at HashTable.h:971 #3 0x02375151 in WTF::HashTable<WebCore::Node*, WTF::KeyValuePair<WebCore::Node*, unsigned int>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::Node*, unsigned int> >, WTF::PtrHash<WebCore::Node*>, WTF::HashMapValueTraits<WTF::HashTraits<WebCore::Node*>, WTF::HashTraits<unsigned int> >, WTF::HashTraits<WebCore::Node*> >::find (this=0xe236760, key=@0xbfff90f8) at HashTable.h:401 #4 0x02374b17 in WTF::HashMap<WebCore::Node*, unsigned int, WTF::PtrHash<WebCore::Node*>, WTF::HashTraits<WebCore::Node*>, WTF::HashTraits<unsigned int> >::find (this=0xe236760, key=@0xbfff90f8) at HashMap.h:291 #5 0x03048411 in WTF::HashCountedSet<WebCore::Node*, WTF::PtrHash<WebCore::Node*>, WTF::HashTraits<WebCore::Node*> >::find (this=0xe236760, value=@0xbfff90f8) at HashCountedSet.h:132 #6 0x0302a263 in WTF::HashCountedSet<WebCore::Node*, WTF::PtrHash<WebCore::Node*>, WTF::HashTraits<WebCore::Node*> >::removeAll (this=0xe236760, value=@0xbfff90f8) at HashCountedSet.h:188 #7 0x0301afb7 in WebCore::Document::didRemoveEventTargetNode (this=0xd91d000, handler=0xd91d000) at ../../third_party/WebKit/Source/WebCore/dom/Document.cpp:5657 #8 0x02d4e73f in WebCore::DOMWindow::~DOMWindow (this=0xe23ff20) at ../../third_party/WebKit/Source/WebCore/page/DOMWindow.cpp:447 #9 0x02d4de3b in WebCore::DOMWindow::~DOMWindow (this=0xe23ff20) at ../../third_party/WebKit/Source/WebCore/page/DOMWindow.cpp:409 #10 0x02d4ddde in WebCore::DOMWindow::~DOMWindow (this=0xe23ff20) at ../../third_party/WebKit/Source/WebCore/page/DOMWindow.cpp:409 #11 0x0316bcce in WTF::RefCounted<WebCore::DOMWindow>::deref (this=0xe23ff24) at RefCounted.h:202 #12 0x0316bc4d in WTF::derefIfNotNull<WebCore::DOMWindow> (ptr=0xe23ff20) at PassRefPtr.h:53 #13 0x030ec55d in WTF::RefPtr<WebCore::DOMWindow>::~RefPtr (this=0xd91d16c) at RefPtr.h:56 #14 0x030ec50b in WTF::RefPtr<WebCore::DOMWindow>::~RefPtr (this=0xd91d16c) at RefPtr.h:56 #15 0x02fff5f2 in WebCore::Document::~Document (this=0xd91d000) at ../../third_party/WebKit/Source/WebCore/dom/Document.cpp:672 #16 0x015bf046 in WebCore::HTMLDocument::~HTMLDocument (this=0xd91d000) at ../../third_party/WebKit/Source/WebCore/html/HTMLDocument.cpp:91 #17 0x015bee6b in WebCore::HTMLDocument::~HTMLDocument (this=0xd91d000) at ../../third_party/WebKit/Source/WebCore/html/HTMLDocument.cpp:90 #18 0x015bee0e in WebCore::HTMLDocument::~HTMLDocument (this=0xd91d000) at ../../third_party/WebKit/Source/WebCore/html/HTMLDocument.cpp:90 #19 0x03144cc5 in WebCore::Document::guardDeref (this=0xd91d000) at Document.h:248 #20 0x031368bc in WebCore::Node::~Node (this=0xe236a00) at ../../third_party/WebKit/Source/WebCore/dom/Node.cpp:436 #21 0x02fd6484 in WebCore::ContainerNode::~ContainerNode (this=0xe236a00) at ../../third_party/WebKit/Source/WebCore/dom/ContainerNode.cpp:126 #22 0x030b2b18 in WebCore::Element::~Element (this=0xe236a00) at ../../third_party/WebKit/Source/WebCore/dom/Element.cpp:205 #23 0x031ebab2 in WebCore::StyledElement::~StyledElement (this=0xe236a00) at ../../third_party/WebKit/Source/WebCore/dom/StyledElement.cpp:142 #24 0x0165e84b in WebCore::HTMLElement::~HTMLElement (this=0xe236a00) at HTMLElement.h:45 #25 0x015a384b in WebCore::HTMLBodyElement::~HTMLBodyElement (this=0xe236a00) at ../../third_party/WebKit/Source/WebCore/html/HTMLBodyElement.cpp:62 #26 0x015a37fb in WebCore::HTMLBodyElement::~HTMLBodyElement (this=0xe236a00) at ../../third_party/WebKit/Source/WebCore/html/HTMLBodyElement.cpp:61 #27 0x015a379e in WebCore::HTMLBodyElement::~HTMLBodyElement (this=0xe236a00) at ../../third_party/WebKit/Source/WebCore/html/HTMLBodyElement.cpp:61 #28 0x03143358 in WebCore::Node::removedLastRef (this=0xe236a00) at ../../third_party/WebKit/Source/WebCore/dom/Node.cpp:2602 #29 0x0112d188 in WebCore::TreeShared<WebCore::Node, WebCore::ContainerNode>::deref (this=0xe236a08) at TreeShared.h:81 #30 0x03138cd1 in WebCore::Node::derefEventTarget (this=0xe236a00) at ../../third_party/WebKit/Source/WebCore/dom/Node.cpp:852 #31 0x03116a71 in WebCore::EventTarget::deref (this=0xe236a00) at EventTarget.h:106 #32 0x03116a18 in WTF::derefIfNotNull<WebCore::EventTarget> (ptr=0xe236a00) at PassRefPtr.h:53 #33 0x031169ad in WTF::RefPtr<WebCore::EventTarget>::~RefPtr (this=0xe21f1ac) at RefPtr.h:56 #34 0x0311572b in WTF::RefPtr<WebCore::EventTarget>::~RefPtr (this=0xe21f1ac) at RefPtr.h:56 #35 0x02d92576 in WebCore::Touch::~Touch (this=0xe21f190) at Touch.h:41 #36 0x02d924ab in WebCore::Touch::~Touch (this=0xe21f190) at Touch.h:41 #37 0x02d92449 in WTF::RefCounted<WebCore::Touch>::deref (this=0xe21f190) at RefCounted.h:202 #38 0x039cd31b in WebCore::V8Touch::derefObject (object=0xe21f190) at V8Touch.cpp:223 #39 0x023d0f38 in WebCore::WrapperTypeInfo::derefObject (this=0x643fa84, object=0xe21f190) at WrapperTypeInfo.h:90 #40 0x023d0817 in WebCore::DOMWrapperMap<void>::defaultWeakCallback (value={<v8::Handle<v8::Value>> = {val_ = 0xd9039f0}, <No data fields>}, context=0xe37b234) at DOMWrapperMap.h:103 #41 0x04a07e51 in v8::internal::GlobalHandles::Node::PostGarbageCollectionProcessing (this=0xd9039f0, isolate=0xf08a200, global_handles=0xe06c700) at ../../v8/src/global-handles.cc:244 #42 0x04a05622 in v8::internal::GlobalHandles::PostGarbageCollectionProcessing (this=0xe06c700, collector=v8::internal::MARK_COMPACTOR, tracer=0xbfff99e8) at ../../v8/src/global-handles.cc:634 #43 0x04a1f79f in v8::internal::Heap::PerformGarbageCollection (this=0xf08a208, collector=v8::internal::MARK_COMPACTOR, tracer=0xbfff99e8) at ../../v8/src/heap.cc:968 #44 0x04a1eec9 in v8::internal::Heap::CollectGarbage (this=0xf08a208, space=v8::internal::NEW_SPACE, collector=v8::internal::MARK_COMPACTOR, gc_reason=0x57ba825 "Runtime::PerformGC", collector_reason=0x5794fd5 "promotion limit reached") at ../../v8/src/heap.cc:651 #45 0x04c03e6b in v8::internal::Heap::CollectGarbage (this=0xf08a208, space=v8::internal::NEW_SPACE, gc_reason=0x57ba825 "Runtime::PerformGC") at heap-inl.h:436 #46 0x04d5ca81 in v8::internal::Runtime::PerformGC (result=0xbad0003) at ../../v8/src/runtime.cc:13501 #47 0x2250a45f in ?? () #48 0x225379f9 in ?? () #49 0x225377e4 in ?? () #50 0x2253751b in ?? () #51 0x22520afb in ?? () #52 0x2251db95 in ?? () #53 0x22527005 in ?? () #54 0x22526d11 in ?? () #55 0x22527841 in ?? () #56 0x2252729d in ?? () #57 0x22526d11 in ?? () #58 0x2250e7e1 in ?? () #59 0x225223b2 in ?? () #60 0x225134ea in ?? () #61 0x049b31bc in v8::internal::Invoke (is_construct=false, function={location_ = 0xd0b1474}, receiver={location_ = 0xd0b147c}, argc=1, args=0xbfff9fd0, has_pending_exception=0xbfffa07f) at ../../v8/src/execution.cc:118 #62 0x049b2ae9 in v8::internal::Execution::Call (callable={location_ = 0xd0b1474}, receiver={location_ = 0xd0b1478}, argc=1, argv=0xbfff9fd0, pending_exception=0xbfffa07f, convert_receiver=false) at ../../v8/src/execution.cc:179 #63 0x049b6003 in v8::internal::Execution::InstantiateFunction (data={location_ = 0xd0b2db0}, exc=0xbfffa07f) at ../../v8/src/execution.cc:719 #64 0x048b5347 in v8::FunctionTemplate::GetFunction (this=0xd0b2db0) at ../../v8/src/api.cc:4796 #65 0x0248da62 in WebCore::V8PerContextData::constructorForTypeSlowCase (this=0xe24c3f0, type=0x643e22c) at ../../third_party/WebKit/Source/WebCore/bindings/v8/V8PerContextData.cpp:120 #66 0x035dedcc in WebCore::V8PerContextData::constructorForType (this=0xe24c3f0, type=0x643e22c) at V8PerContextData.h:88 #67 0x0248d6b2 in WebCore::V8PerContextData::createWrapperFromCacheSlowCase (this=0xe24c3f0, type=0x643e22c) at ../../third_party/WebKit/Source/WebCore/bindings/v8/V8PerContextData.cpp:102 #68 0x0246d081 in WebCore::V8PerContextData::createWrapperFromCache (this=0xe24c3f0, type=0x643e22c) at V8PerContextData.h:80 #69 0x0246c4b1 in WebCore::V8DOMWrapper::createWrapper (creationContext={val_ = 0xbfffbd54}, type=0x643e22c, impl=0xe210520) at ../../third_party/WebKit/Source/WebCore/bindings/v8/V8DOMWrapper.cpp:112 #70 0x0379762b in WebCore::V8HTMLParagraphElement::createWrapper (impl=@0xbfffa5f8, creationContext={val_ = 0xbfffbd54}, isolate=0xf08a200) at V8HTMLParagraphElement.cpp:119 #71 0x03a8bbc6 in WebCore::wrap (impl=0xe210520, creationContext={val_ = 0xbfffbd54}, isolate=0xf08a200) at V8HTMLParagraphElement.h:60 #72 0x03a8514b in WebCore::createHTMLParagraphElementWrapper (element=0xe210520, creationContext={val_ = 0xbfffbd54}, isolate=0xf08a200) at gen/webkit/V8HTMLElementWrapperFactory.cpp:583 #73 0x03a83536 in WebCore::createV8HTMLWrapper (element=0xe210520, creationContext={val_ = 0xbfffbd54}, isolate=0xf08a200) at gen/webkit/V8HTMLElementWrapperFactory.cpp:901 #74 0x024eb8a4 in WebCore::wrap (impl=0xe210520, creationContext={val_ = 0xbfffbd54}, isolate=0xf08a200) at ../../third_party/WebKit/Source/WebCore/bindings/v8/custom/V8HTMLElementCustom.cpp:46 #75 0x024d52c4 in WebCore::wrap (impl=0xe210520, creationContext={val_ = 0xbfffbd54}, isolate=0xf08a200) at ../../third_party/WebKit/Source/WebCore/bindings/v8/custom/V8ElementCustom.cpp:48 #76 0x038eae92 in WebCore::toV8Fast<v8::Arguments, WebCore::Document> (impl=0xe210520, container=@0xbfffbd38, wrappable=0xd906c00) at V8Element.h:78 #77 0x03889394 in WebCore::DocumentV8Internal::getElementByIdCallback (args=@0xbfffbd38) at V8Document.cpp:1439 #78 0x04933986 in v8::internal::HandleApiCallHelper<false> (args={<v8::internal::Arguments> = {<v8::internal::Embedded> = {<No data fields>}, length_ = 3, arguments_ = 0xbfffbe94}, <No data fields>}, isolate=0xf08a200) at ../../v8/src/builtins.cc:1372 #79 0x0493342a in v8::internal::Builtin_Impl_HandleApiCall (args={<v8::internal::Arguments> = {<v8::internal::Embedded> = {<No data fields>}, length_ = 3, arguments_ = 0xbfffbe94}, <No data fields>}, isolate=0xf08a200) at ../../v8/src/builtins.cc:1390 #80 0x049284cc in v8::internal::Builtin_HandleApiCall (args={<v8::internal::Arguments> = {<v8::internal::Embedded> = {<No data fields>}, length_ = 3, arguments_ = 0xbfffbe94}, <No data fields>}, isolate=0xf08a200) at ../../v8/src/builtins.cc:1389 #81 0x2250a3f6 in ?? () #82 0x22539e35 in ?? () #83 0x22539ca6 in ?? () #84 0x2250e7e1 in ?? () #85 0x45c71ebf in ?? () #86 0x45c71c57 in ?? () #87 0x225223b9 in ?? () #88 0x225134ea in ?? () #89 0x049b31bc in v8::internal::Invoke (is_construct=false, function={location_ = 0xd0b144c}, receiver={location_ = 0xd0b145c}, argc=1, args=0xbfffc408, has_pending_exception=0xbfffc1bb) at ../../v8/src/execution.cc:118 #90 0x049b2ae9 in v8::internal::Execution::Call (callable={location_ = 0xd0b144c}, receiver={location_ = 0xd0b145c}, argc=1, argv=0xbfffc408, pending_exception=0xbfffc1bb, convert_receiver=false) at ../../v8/src/execution.cc:179 #91 0x048afcf9 in v8::Function::Call (this=0xd0b144c, recv={val_ = 0xd0b145c}, argc=1, argv=0xbfffc408) at ../../v8/src/api.cc:3785 #92 0x024055d1 in WebCore::ScriptController::callFunctionWithInstrumentation (context=0xd906c54, function={val_ = 0xd0b144c}, receiver={val_ = 0xd0b145c}, argc=1, args=0xbfffc408) at ../../third_party/WebKit/Source/WebCore/bindings/v8/ScriptController.cpp:233 #93 0x02405272 in WebCore::ScriptController::callFunction (this=0xf09887c, function={val_ = 0xd0b144c}, receiver={val_ = 0xd0b145c}, argc=1, args=0xbfffc408) at ../../third_party/WebKit/Source/WebCore/bindings/v8/ScriptController.cpp:186 #94 0x02479334 in WebCore::V8LazyEventListener::callListenerFunction (this=0xe20f110, context=0xd906c54, jsEvent={val_ = 0xd0b1434}, event=0xe2210b0) at ../../third_party/WebKit/Source/WebCore/bindings/v8/V8LazyEventListener.cpp:103 #95 0x0245e050 in WebCore::V8AbstractEventListener::invokeEventHandler (this=0xe20f110, context=0xd906c54, event=0xe2210b0, jsEvent={val_ = 0xd0b1434}) at ../../third_party/WebKit/Source/WebCore/bindings/v8/V8AbstractEventListener.cpp:142 #96 0x0245dcd7 in WebCore::V8AbstractEventListener::handleEvent (this=0xe20f110, context=0xd906c54, event=0xe2210b0) at ../../third_party/WebKit/Source/WebCore/bindings/v8/V8AbstractEventListener.cpp:102 #97 0x030f669e in WebCore::EventTarget::fireEventListeners (this=0xe2160f0, event=0xe2210b0, d=0xe216190, entry=@0xe232450) at ../../third_party/WebKit/Source/WebCore/dom/EventTarget.cpp:210 #98 0x030f640d in WebCore::EventTarget::fireEventListeners (this=0xe2160f0, event=0xe2210b0) at ../../third_party/WebKit/Source/WebCore/dom/EventTarget.cpp:175 #99 0x02d4cb0f in WebCore::DOMWindow::dispatchEvent (this=0xe2160f0, prpEvent=@0xbfffc9b0, prpTarget=@0xbfffc9a8) at ../../third_party/WebKit/Source/WebCore/page/DOMWindow.cpp:1675 #100 0x02d557a7 in WebCore::DOMWindow::dispatchLoadEvent (this=0xe2160f0) at ../../third_party/WebKit/Source/WebCore/page/DOMWindow.cpp:1649 #101 0x0300b23b in WebCore::Document::dispatchWindowLoadEvent (this=0xd906c00) at ../../third_party/WebKit/Source/WebCore/dom/Document.cpp:3658 #102 0x03008233 in WebCore::Document::implicitClose (this=0xd906c00) at ../../third_party/WebKit/Source/WebCore/dom/Document.cpp:2422 #103 0x02c274c2 in WebCore::FrameLoader::checkCallImplicitClose (this=0xf098460) at ../../third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:833 #104 0x02c2703e in WebCore::FrameLoader::checkCompleted (this=0xf098460) at ../../third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:776 #105 0x02c2727b in WebCore::FrameLoader::loadDone (this=0xf098460) at ../../third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:722 #106 0x02ce162c in WebCore::CachedResourceLoader::loadDone (this=0xe21bd80, resource=0xd8eee00) at ../../third_party/WebKit/Source/WebCore/loader/cache/CachedResourceLoader.cpp:721 #107 0x02c8c565 in WebCore::SubresourceLoader::releaseResources (this=0xd8fa200) at ../../third_party/WebKit/Source/WebCore/loader/SubresourceLoader.cpp:318 #108 0x02c838f3 in WebCore::ResourceLoader::didFinishLoading (this=0xd8fa200, finishTime=0) at ../../third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:317 #109 0x02c8bffc in WebCore::SubresourceLoader::didFinishLoading (this=0xd8fa200, finishTime=0) at ../../third_party/WebKit/Source/WebCore/loader/SubresourceLoader.cpp:277 #110 0x02c8435f in WebCore::ResourceLoader::didFinishLoading (this=0xd8fa200, finishTime=0) at ../../third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:456 #111 0x0209331a in WebCore::ResourceHandleInternal::didFinishLoading (this=0xe229fc0, finishTime=0) at ../../third_party/WebKit/Source/WebCore/platform/network/chromium/ResourceHandle.cpp:156 #112 0x0196ce2e in webkit_glue::WebURLLoaderImpl::Context::OnCompletedRequest (this=0xe220a90, error_code=0, was_ignored_by_handler=false, security_info=@0xc489d1c, completion_time=@0xc489d20) at ../../webkit/glue/weburlloader_impl.cc:673 #113 0x02f8f02d in (anonymous namespace)::RequestProxy::NotifyCompletedRequest (this=0xe221310, error_code=0, security_info=@0xc489d1c, complete_time=@0xc489d20) at ../../webkit/tools/test_shell/simple_resource_loader_bridge.cc:404 #114 0x02f8f847 in base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(int, std::string const&, base::TimeTicks const&)>::Run (this=0xbfffd080, object=0xe221310, a1=@0xc489d18, a2=@0xc489d1c, a3=@0xc489d20) at bind_internal.h:311 #115 0x02f8f746 in base::internal::InvokeHelper<false, void, base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(int, std::string const&, base::TimeTicks const&)>, void ()((anonymous namespace)::RequestProxy* const&, int const&, std::string const&, base::TimeTicks const&)>::MakeItSo (runnable={method_ = {ptr = 49868720, ptr = 0}}, a1=@0xc489d14, a2=@0xc489d18, a3=@0xc489d1c, a4=@0xc489d20) at bind_internal.h:960 #116 0x02f8f685 in base::internal::Invoker<4, base::internal::BindState<base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(int, std::string const&, base::TimeTicks const&)>, void ()((anonymous namespace)::RequestProxy*, int, std::string const&, base::TimeTicks const&), void ()((anonymous namespace)::RequestProxy*, int, std::string, base::TimeTicks)>, void ()((anonymous namespace)::RequestProxy*, int, std::string const&, base::TimeTicks const&)>::Run (base=0xc489d00) at bind_internal.h:1572 #117 0x012b261b in base::Callback<void ()()>::Run (this=0xbfffd354) at callback.h:396 #118 0x012afb5b in MessageLoop::RunTask (this=0xe06a620, pending_task=@0xbfffd340) at ../../base/message_loop.cc:473 #119 0x012b0022 in MessageLoop::DeferOrRunPendingTask (this=0xe06a620, pending_task=@0xbfffd340) at ../../base/message_loop.cc:485 #120 0x012b0222 in MessageLoop::DoWork (this=0xe06a620) at ../../base/message_loop.cc:668 #121 0x01218dcb in base::MessagePumpCFRunLoopBase::RunWork (this=0xe069c90) at ../../base/message_pump_mac.mm:250 #122 0x01218582 in base::MessagePumpCFRunLoopBase::RunWorkSource (info=0xe069c90) at ../../base/message_pump_mac.mm:228 #123 0x982cc66f in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ () #124 0x982cc099 in __CFRunLoopDoSources0 () #125 0x982f1e46 in __CFRunLoopRun () #126 0x982f163a in CFRunLoopRunSpecific () #127 0x982f14ab in CFRunLoopRunInMode () #128 0x9a39115a in RunCurrentEventLoopInMode () #129 0x9a390ec9 in ReceiveNextEventCommon () #130 0x9a390d44 in BlockUntilNextEventMatchingListInMode () #131 0x96c4da3a in _DPSNextEvent () #132 0x96c4d26c in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] () #133 0x96c436cc in -[NSApplication run] () #134 0x01219cfe in base::MessagePumpNSApplication::DoRun (this=0xe069c90, delegate=0xe06a620) at ../../base/message_pump_mac.mm:574 #135 0x01218b38 in base::MessagePumpCFRunLoopBase::Run (this=0xe069c90, delegate=0xe06a620) at ../../base/message_pump_mac.mm:169 #136 0x012af3a2 in MessageLoop::RunInternal (this=0xe06a620) at ../../base/message_loop.cc:430 #137 0x012af25b in MessageLoop::RunHandler (this=0xe06a620) at ../../base/message_loop.cc:403 #138 0x0130c6c8 in base::RunLoop::Run (this=0xbfffecf8) at ../../base/run_loop.cc:45 #139 0x012ae656 in MessageLoop::Run (this=0xe06a620) at ../../base/message_loop.cc:310 #140 0x046ad407 in webkit_support::RunMessageLoop () at ../../webkit/support/webkit_support.cc:553 #141 0x00062f39 in TestShell::waitTestFinished (this=0xbffff798) at ../../third_party/WebKit/Tools/DumpRenderTree/chromium/TestShellMac.mm:122 #142 0x00050949 in TestShell::runFileTest (this=0xbffff798, params=@0xbffff9f0, shouldDumpPixels=false) at ../../third_party/WebKit/Tools/DumpRenderTree/chromium/TestShell.cpp:294 #143 0x0000700f in runTest (shell=@0xbffff798, params=@0xbffff9f0, inputLine=@0xbfffef88, forceDumpPixels=false) at ../../third_party/WebKit/Tools/DumpRenderTree/chromium/DumpRenderTree.cpp:115 #144 0x00006b07 in main (argc=2, argv=0xbffffa90) at ../../third_party/WebKit/Tools/DumpRenderTree/chromium/DumpRenderTree.cpp:268
Levi Weintraub
Comment 59
2012-12-15 19:20:47 PST
(In reply to
comment #58
)
> Definitely crashing - not every time, but here's one example stack from fast/events/touch/
Thanks for the bt. I'm taking a look.
Levi Weintraub
Comment 60
2012-12-16 19:08:10 PST
Created
attachment 179682
[details]
Patch
James Robinson
Comment 61
2012-12-16 23:28:54 PST
Comment on
attachment 179682
[details]
Patch If I'm following the diffs correctly, this moved some cleanup logic from ~DOMWindow() to ~Document() since in ~DOMWindow() it ran as Document's members were being destroyed and the touch set was destroyed before ~DOMWindow() ran - is that correct? Why do we add a document to its parent's touch set at all when handlers are registered inside it? It looks like accumulateDocumentEventTargetRects() will recursively check child documents.
Levi Weintraub
Comment 62
2012-12-17 01:13:43 PST
(In reply to
comment #61
)
> (From update of
attachment 179682
[details]
) > If I'm following the diffs correctly, this moved some cleanup logic from ~DOMWindow() to ~Document() since in ~DOMWindow() it ran as Document's members were being destroyed and the touch set was destroyed before ~DOMWindow() ran - is that correct?
That is correct.
> > Why do we add a document to its parent's touch set at all when handlers are registered inside it? It looks like accumulateDocumentEventTargetRects() will recursively check child documents.
We add the child Document to the parent Document's list so the main frame's Document can ultimately track the list, as that Document is tied to the Page and ScrollingCoordinator, which generates the rects.
Levi Weintraub
Comment 63
2012-12-17 09:11:04 PST
(In reply to
comment #62
)
> (In reply to
comment #61
) > > Why do we add a document to its parent's touch set at all when handlers are registered inside it? It looks like accumulateDocumentEventTargetRects() will recursively check child documents. > > We add the child Document to the parent Document's list so the main frame's Document can ultimately track the list, as that Document is tied to the Page and ScrollingCoordinator, which generates the rects.
We only check child documents that are in the parent's TouchEventTarget set.
James Robinson
Comment 64
2012-12-17 13:16:15 PST
Comment on
attachment 179682
[details]
Patch R=me
Levi Weintraub
Comment 65
2012-12-17 13:17:51 PST
Comment on
attachment 179682
[details]
Patch Thanks for taking a look!
WebKit Review Bot
Comment 66
2012-12-17 13:43:33 PST
Comment on
attachment 179682
[details]
Patch Clearing flags on attachment: 179682 Committed
r137939
: <
http://trac.webkit.org/changeset/137939
>
WebKit Review Bot
Comment 67
2012-12-17 13:43:43 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug