Bug 89629 – Use Dictionary in MutationObserver.observe to kill custom code

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

Use Dictionary in MutationObserver.observe to kill custom code

[Adam Klein]

Created attachment 148707 [details]
Patch

[Kentaro Hara]

Comment on attachment 148707 [details]
Patch

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

> Source/WebCore/dom/WebKitMutationObserver.cpp:95
> +    if (optionsDictionary.isUndefinedOrNull()) {
> +        ec = TYPE_MISMATCH_ERR;
> +        return;
> +    }

I guess that this check would be already done in the generated code before WebKitMutationObserver::observe() is called back.

[Adam Klein]

(In reply to comment #2)
> (From update of attachment 148707 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148707&action=review
> 
> > Source/WebCore/dom/WebKitMutationObserver.cpp:95
> > +    if (optionsDictionary.isUndefinedOrNull()) {
> > +        ec = TYPE_MISMATCH_ERR;
> > +        return;
> > +    }
> 
> I guess that this check would be already done in the generated code before WebKitMutationObserver::observe() is called back.

It's not. If I leave this out, I get the SYNTAX_ERR due to validateOptions failing (this is covered by fast/mutation/observe-exceptions.html).  Per WebIDL, it seems we're actually supposed to throw a TypeError if the passed-in dictionary isn't an Object:

http://www.w3.org/TR/WebIDL/#es-dictionary

The code as I've written it is maintains the pre-existing behavior.

[Adam Klein]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 148707 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=148707&action=review
> > 
> > > Source/WebCore/dom/WebKitMutationObserver.cpp:95
> > > +    if (optionsDictionary.isUndefinedOrNull()) {
> > > +        ec = TYPE_MISMATCH_ERR;
> > > +        return;
> > > +    }
> > 
> > I guess that this check would be already done in the generated code before WebKitMutationObserver::observe() is called back.
> 
> It's not. If I leave this out, I get the SYNTAX_ERR due to validateOptions failing (this is covered by fast/mutation/observe-exceptions.html).  Per WebIDL, it seems we're actually supposed to throw a TypeError if the passed-in dictionary isn't an Object:
> 
> http://www.w3.org/TR/WebIDL/#es-dictionary
> 
> The code as I've written it is maintains the pre-existing behavior.

Hmm, but you're quite right, the generated code does try to do this check. Investigating further...

[Adam Klein]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 148707 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=148707&action=review
> > > 
> > > > Source/WebCore/dom/WebKitMutationObserver.cpp:95
> > > > +    if (optionsDictionary.isUndefinedOrNull()) {
> > > > +        ec = TYPE_MISMATCH_ERR;
> > > > +        return;
> > > > +    }
> > > 
> > > I guess that this check would be already done in the generated code before WebKitMutationObserver::observe() is called back.
> > 
> > It's not. If I leave this out, I get the SYNTAX_ERR due to validateOptions failing (this is covered by fast/mutation/observe-exceptions.html).  Per WebIDL, it seems we're actually supposed to throw a TypeError if the passed-in dictionary isn't an Object:
> > 
> > http://www.w3.org/TR/WebIDL/#es-dictionary
> > 
> > The code as I've written it is maintains the pre-existing behavior.
> 
> Hmm, but you're quite right, the generated code does try to do this check. Investigating further...

Oh, I just misread the generated code. It passes on undefined or null options, apparently it only catches the case of non-Object, non-null, non-undefined:

    if (args.Length() > 1 && !options.isUndefinedOrNull() && !options.isObject()) {
        ec = TYPE_MISMATCH_ERR;
        V8Proxy::setDOMException(ec, args.GetIsolate());
        return V8Proxy::throwTypeError("Not an object.");
    }

Should we change CodeGenerator to do something different here?

[Kentaro Hara]

(In reply to comment #5)
> Oh, I just misread the generated code. It passes on undefined or null options, apparently it only catches the case of non-Object, non-null, non-undefined:
> 
>     if (args.Length() > 1 && !options.isUndefinedOrNull() && !options.isObject()) {
>         ec = TYPE_MISMATCH_ERR;
>         V8Proxy::setDOMException(ec, args.GetIsolate());
>         return V8Proxy::throwTypeError("Not an object.");
>     }
> 
> Should we change CodeGenerator to do something different here?

Yeah, fixing the condition sounds reasonable.

Also, per the spec, we do not need to set TYPE_MISMATCH_ERR, we just need to throw TypeError. (Note: TYPE_MISMATCH_ERR and TypeError are different things.) If there is no concern about compatibility, we might want to remove TYPE_MISMATCH_ERR.

[Adam Klein]

(In reply to comment #6)
> (In reply to comment #5)
> > Oh, I just misread the generated code. It passes on undefined or null options, apparently it only catches the case of non-Object, non-null, non-undefined:
> > 
> >     if (args.Length() > 1 && !options.isUndefinedOrNull() && !options.isObject()) {
> >         ec = TYPE_MISMATCH_ERR;
> >         V8Proxy::setDOMException(ec, args.GetIsolate());
> >         return V8Proxy::throwTypeError("Not an object.");
> >     }
> > 
> > Should we change CodeGenerator to do something different here?
> 
> Yeah, fixing the condition sounds reasonable.

Okay.

> Also, per the spec, we do not need to set TYPE_MISMATCH_ERR, we just need to throw TypeError. (Note: TYPE_MISMATCH_ERR and TypeError are different things.) If there is no concern about compatibility, we might want to remove TYPE_MISMATCH_ERR.

Yeah, I know they're different, but I don't think there are compat concerns here.

I'd like to do both of the aforementioned things in separate changes, though, since they both could change behavior. This patch does not change behavior as-is.

[Kentaro Hara]

(In reply to comment #6)
> >     if (args.Length() > 1 && !options.isUndefinedOrNull() && !options.isObject()) 

This condition looks really strange. Maybe it could be:

    if (options.isUndefinedOrNull() || options.isObject())

I think we do not need to check args.Length(), since it is already handled by MAYBE_MISSING_PARAMETER() macro.

[WebKit Review Bot]

Comment on attachment 148707 [details]
Patch

Clearing flags on attachment: 148707

Committed r120900: <http://trac.webkit.org/changeset/120900>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.