Bug 80549 - [MutationObservers] Enforce a consistent order of MutationRecord delivery
Summary: [MutationObservers] Enforce a consistent order of MutationRecord delivery
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
: 79710 (view as bug list)
Depends on:
Blocks: 68729
  Show dependency treegraph
 
Reported: 2012-03-07 15:42 PST by Adam Klein
Modified: 2012-03-21 13:49 PDT (History)
5 users (show)

See Also:


Attachments
Patch (11.09 KB, patch)
2012-03-07 15:45 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (11.08 KB, patch)
2012-03-12 12:18 PDT, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2012-03-07 15:42:26 PST
[MutationObservers] Enforce a consistent order of MutationRecord delivery
Comment 1 Adam Klein 2012-03-07 15:45:11 PST
Created attachment 130713 [details]
Patch
Comment 2 Adam Klein 2012-03-07 15:48:21 PST
The exact ordering is currently specified in http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-mo-invoke, but I've suggested a change to the algorithm to match this patch in http://lists.w3.org/Archives/Public/www-dom/2012JanMar/0145.html.
Comment 3 Adam Klein 2012-03-12 11:04:55 PDT
This is now ready for review.
Comment 4 Ojan Vafai 2012-03-12 11:17:12 PDT
Comment on attachment 130713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130713&action=review

> Source/WebCore/dom/WebKitMutationObserver.h:98
> +    int m_priority;

Are you at all worried about overflow? At the least, this should probably be unsigned, no?
Comment 5 Adam Klein 2012-03-12 11:23:12 PDT
Comment on attachment 130713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130713&action=review

>> Source/WebCore/dom/WebKitMutationObserver.h:98
>> +    int m_priority;
> 
> Are you at all worried about overflow? At the least, this should probably be unsigned, no?

I find it unlikely that a renderer would behave reasonably having created 2 billion mutation observers. Making it unsigned doesn't help much (if you've already created 2 billion, why not another 2 billion?).

If you think this is something that we should worry about, perhaps a completely different implementation (something better matching the spec, perhaps) could be in order: keeping a list of all living mutation observers, and iterating over that for delivery. But that will behave poorly if we have lots of observers only a few of which are active at a time.
Comment 6 Adam Klein 2012-03-12 12:11:42 PDT
Comment on attachment 130713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130713&action=review

>>> Source/WebCore/dom/WebKitMutationObserver.h:98
>>> +    int m_priority;
>> 
>> Are you at all worried about overflow? At the least, this should probably be unsigned, no?
> 
> I find it unlikely that a renderer would behave reasonably having created 2 billion mutation observers. Making it unsigned doesn't help much (if you've already created 2 billion, why not another 2 billion?).
> 
> If you think this is something that we should worry about, perhaps a completely different implementation (something better matching the spec, perhaps) could be in order: keeping a list of all living mutation observers, and iterating over that for delivery. But that will behave poorly if we have lots of observers only a few of which are active at a time.

As per offline discussion, making this unsigned and punting overflow.
Comment 7 Adam Klein 2012-03-12 12:18:13 PDT
Created attachment 131378 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-03-12 13:11:33 PDT
Comment on attachment 131378 [details]
Patch for landing

Clearing flags on attachment: 131378

Committed r110465: <http://trac.webkit.org/changeset/110465>
Comment 9 WebKit Review Bot 2012-03-12 13:11:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Adam Klein 2012-03-21 13:49:34 PDT
*** Bug 79710 has been marked as a duplicate of this bug. ***