Bug 97372 - Remove bogus assertions from ChildListMutationScope
Summary: Remove bogus assertions from ChildListMutationScope
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:
Depends on: 97352
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-21 15:36 PDT by Adam Klein
Modified: 2012-09-21 18:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (9.04 KB, patch)
2012-09-21 15:46 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (9.16 KB, patch)
2012-09-21 17:00 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-09-21 15:36:52 PDT
Remove bogus assertions from ChildListMutationScope
Comment 1 Adam Klein 2012-09-21 15:46:47 PDT
Created attachment 165215 [details]
Patch
Comment 2 Adam Klein 2012-09-21 15:47:48 PDT
This patch is against 97352, so it's not ready for EWS yet.
Comment 3 Ryosuke Niwa 2012-09-21 16:06:22 PDT
Comment on attachment 165215 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Some asserts (and their accompanying comment) were trying to enforce
> +        proper usage of ChildListMutationScope from WebCore, but in the
> +        presence of MutationEvents they could fail due to arbitrary script
> +        execution.

Does this invalidate the assumption stated above the declaration of MutationAccumulator?
(I suppose that's the case given you're removing the comment).
In the presence of mutation events, can we end up delivering mutation records before the outmost scope closes?

If so, are we violating the specification in this regard?
Comment 4 Adam Klein 2012-09-21 16:10:52 PDT
Comment on attachment 165215 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        execution.
> 
> Does this invalidate the assumption stated above the declaration of MutationAccumulator?
> (I suppose that's the case given you're removing the comment).
> In the presence of mutation events, can we end up delivering mutation records before the outmost scope closes?
> 
> If so, are we violating the specification in this regard?

We don't deliver early, just enqueue early. Since the spec doesn't mention the existence of MutationEvents, it's hard to say whether we're violating the specification. But we're doing something sane (MutationObservers still report everything that happens), as documented in these test cases.
Comment 5 Ryosuke Niwa 2012-09-21 16:15:03 PDT
(In reply to comment #4)
>
> We don't deliver early, just enqueue early. Since the spec doesn't mention the existence of MutationEvents, it's hard to say whether we're violating the specification. But we're doing something sane (MutationObservers still report everything that happens), as documented in these test cases.

Okay. Does our behavior match that of Gecko? I think it'll be nice to at least touch-base with Jonas/Olli to make sure they're okay with our behavior. Given that there are quite few js libraries still relying on mutation events (notably rich text editors), we need to make sure the way our behavior doesn't adversely affect websites that include those editors.
Comment 6 Adam Klein 2012-09-21 16:18:04 PDT
(In reply to comment #5)
> (In reply to comment #4)
> >
> > We don't deliver early, just enqueue early. Since the spec doesn't mention the existence of MutationEvents, it's hard to say whether we're violating the specification. But we're doing something sane (MutationObservers still report everything that happens), as documented in these test cases.
> 
> Okay. Does our behavior match that of Gecko? I think it'll be nice to at least touch-base with Jonas/Olli to make sure they're okay with our behavior. Given that there are quite few js libraries still relying on mutation events (notably rich text editors), we need to make sure the way our behavior doesn't adversely affect websites that include those editors.

Not sure what you'd want to ask. The particular records emitted seem not terribly important in this case, as long as they're an accurate portrayal of the current state of the DOM. Without a spec (which we're not going to get, because MutationEvents will not be specced in DOM4), I don't think exact interoperability is something we should strive for (since it wouldn't be written down anywhere other than in an email). It's just an implementation detail.
Comment 7 Ryosuke Niwa 2012-09-21 16:31:06 PDT
I just chatted with Jonas and confirmed that he doesn't really care about this case.
Comment 8 Adam Klein 2012-09-21 17:00:29 PDT
Created attachment 165228 [details]
Patch
Comment 9 WebKit Review Bot 2012-09-21 18:27:32 PDT
Comment on attachment 165228 [details]
Patch

Clearing flags on attachment: 165228

Committed r129288: <http://trac.webkit.org/changeset/129288>
Comment 10 WebKit Review Bot 2012-09-21 18:27:36 PDT
All reviewed patches have been landed.  Closing bug.