Bug 151165 – jQuery 'is' implementation causes exception to be thrown

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Brent Fulgham]

Created attachment 265332 [details]
Example that triggers the issue.

While debugging another issue, I noticed that exceptions were being thrown inside JSC in a fairly simple jQuery test page. These 'exceptions' are not catchable in script, and seem to be consumed inside WebKit without triggering any external exception handling behavior. Perhaps this is normal and expected, but I'm writing this bug up so we can make that decision.

Consider the following code:

    if (!elem.is(":visible")) {
        ... do something
    }

When this code is executed (at least in jQuery 2.1.4), we are encountering a DOMError 12 (Syntax Error) being generated in the WebCore::JsElementPrototypeFunctionMatches method.

+  377024 ( 408808 -  31784)    373 allocsBackTrace175CD6B0
+     344 (    373 -     29)BackTrace175CD6B0allocations

    ntdll!RtlpCallInterceptRoutine+26
    ntdll!RtlReAllocateHeap+3E7B1
    WTF!realloc+43 (f:\dd\vctools\crt\crtw32\heap\realloc.c, 85)
    WTF!WTF::fastRealloc+E (c:\bwa\wtf-7601.2.3\srcroot\wtf\fastmalloc.cpp, 155)
    WTF!WTF::StringImpl::reallocateInternal<unsigned char>+72 (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringimpl.cpp, 224)
    WTF!WTF::StringImpl::reallocate+23 (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringimpl.cpp, 231)
    WTF!WTF::StringBuilder::reallocateBuffer<unsigned char>+4F (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringbuilder.cpp, 150)
    WTF!WTF::StringBuilder::appendUninitializedSlow<unsigned char>+3C (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringbuilder.cpp, 235)
    WTF!WTF::StringBuilder::append+73 (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringbuilder.cpp, 283)
    JavaScriptCore!JSC::Interpreter::stackTraceAsString+C5 (c:\bwa\javascriptcore-7601.2.3\srcroot\interpreter\interpreter.cpp, 580)
    JavaScriptCore!JSC::addErrorInfoAndGetBytecodeOffset+2D2 (c:\bwa\javascriptcore-7601.2.3\srcroot\runtime\error.cpp, 179)
    JavaScriptCore!JSC::addErrorInfo+39 (c:\bwa\javascriptcore-7601.2.3\srcroot\runtime\error.cpp, 190)
    WebKit!WebCore::createDOMException+273
    WebKit!WebCore::setDOMException+4B
    WebKit!WebCore::jsElementPrototypeFunctionMatches+12A

The jsElementPrototypeFunctionMatches implementation calls the Element::matches method, which eventually takes us to SelectorQueryCache::add:

    SelectorQuery* SelectorQueryCache::add(const String& selectors, Document& document, ExceptionCode& ec)
    SelectorQuery* Document::selectorQueryForString(const String& selectorString, ExceptionCode& ec)
    bool Element::matches(const String& selector, ExceptionCode& ec)

When SelectorQueryCache::add parses the document looking for selectors, and does not find a match the exception code is set to 12 (SYNTAX_ERR) and it does an early return.

This returns a false value, and everyone is happy. Except, the presence of the SYNTAX_ERR code causes us to generate an exception objects:

    JSValue result = jsBoolean(impl.matches(selectors, ec));

    setDOMException(state, ec);
    return JSValue::encode(result);

The call to 'setDOMException' creates an exception object, which lives in the JSC heap until it gets cleaned up. The exception object is fairly heavy-weight, since it contains a call stack and other information about the error location. In some jQuery applications we measured, this can produce a few Megabytes of memory use within a short period.

I'm wondering if we really do want to be treating this 'normal' matches case as an exception.

[Brent Fulgham]

Note: The exception information created by setDOMException is lost in this case. No exception is thrown, and cannot be caught at the JavaScript level.

[Geoffrey Garen]

What do other browsers do?

What does the spec say?

[Chris Dumez]

(In reply to comment #0)
> Created attachment 265332 [details]
> Example that triggers the issue.
> 
> While debugging another issue, I noticed that exceptions were being thrown
> inside JSC in a fairly simple jQuery test page. These 'exceptions' are not
> catchable in script, and seem to be consumed inside WebKit without
> triggering any external exception handling behavior. Perhaps this is normal
> and expected, but I'm writing this bug up so we can make that decision.
> 
> Consider the following code:
> 
>     if (!elem.is(":visible")) {
>         ... do something
>     }
> 
> When this code is executed (at least in jQuery 2.1.4), we are encountering a
> DOMError 12 (Syntax Error) being generated in the
> WebCore::JsElementPrototypeFunctionMatches method.
> 
> +  377024 ( 408808 -  31784)    373 allocsBackTrace175CD6B0
> +     344 (    373 -     29)BackTrace175CD6B0allocations
> 
>     ntdll!RtlpCallInterceptRoutine+26
>     ntdll!RtlReAllocateHeap+3E7B1
>     WTF!realloc+43 (f:\dd\vctools\crt\crtw32\heap\realloc.c, 85)
>     WTF!WTF::fastRealloc+E (c:\bwa\wtf-7601.2.3\srcroot\wtf\fastmalloc.cpp,
> 155)
>     WTF!WTF::StringImpl::reallocateInternal<unsigned char>+72
> (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringimpl.cpp, 224)
>     WTF!WTF::StringImpl::reallocate+23
> (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringimpl.cpp, 231)
>     WTF!WTF::StringBuilder::reallocateBuffer<unsigned char>+4F
> (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringbuilder.cpp, 150)
>     WTF!WTF::StringBuilder::appendUninitializedSlow<unsigned char>+3C
> (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringbuilder.cpp, 235)
>     WTF!WTF::StringBuilder::append+73
> (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringbuilder.cpp, 283)
>     JavaScriptCore!JSC::Interpreter::stackTraceAsString+C5
> (c:\bwa\javascriptcore-7601.2.3\srcroot\interpreter\interpreter.cpp, 580)
>     JavaScriptCore!JSC::addErrorInfoAndGetBytecodeOffset+2D2
> (c:\bwa\javascriptcore-7601.2.3\srcroot\runtime\error.cpp, 179)
>     JavaScriptCore!JSC::addErrorInfo+39
> (c:\bwa\javascriptcore-7601.2.3\srcroot\runtime\error.cpp, 190)
>     WebKit!WebCore::createDOMException+273
>     WebKit!WebCore::setDOMException+4B
>     WebKit!WebCore::jsElementPrototypeFunctionMatches+12A
> 
> The jsElementPrototypeFunctionMatches implementation calls the
> Element::matches method, which eventually takes us to
> SelectorQueryCache::add:
> 
>     SelectorQuery* SelectorQueryCache::add(const String& selectors,
> Document& document, ExceptionCode& ec)
>     SelectorQuery* Document::selectorQueryForString(const String&
> selectorString, ExceptionCode& ec)
>     bool Element::matches(const String& selector, ExceptionCode& ec)
> 
> When SelectorQueryCache::add parses the document looking for selectors, and
> does not find a match the exception code is set to 12 (SYNTAX_ERR) and it
> does an early return.
> 
> This returns a false value, and everyone is happy. Except, the presence of
> the SYNTAX_ERR code causes us to generate an exception objects:
> 
>     JSValue result = jsBoolean(impl.matches(selectors, ec));
> 
>     setDOMException(state, ec);
>     return JSValue::encode(result);
> 
> The call to 'setDOMException' creates an exception object, which lives in
> the JSC heap until it gets cleaned up. The exception object is fairly
> heavy-weight, since it contains a call stack and other information about the
> error location. In some jQuery applications we measured, this can produce a
> few Megabytes of memory use within a short period.
> 
> I'm wondering if we really do want to be treating this 'normal' matches case
> as an exception.

If we throw, this likely means that we were unable to parse this selector. Throwing in such case is as per the spec. However, I am not familiar enough with this selector to know if it is valid.

[Chris Dumez]

(In reply to comment #0)
> Created attachment 265332 [details]
> Example that triggers the issue.
> 
> While debugging another issue, I noticed that exceptions were being thrown
> inside JSC in a fairly simple jQuery test page. These 'exceptions' are not
> catchable in script, and seem to be consumed inside WebKit without
> triggering any external exception handling behavior. Perhaps this is normal
> and expected, but I'm writing this bug up so we can make that decision.
> 
> Consider the following code:
> 
>     if (!elem.is(":visible")) {
>         ... do something
>     }
> 
> When this code is executed (at least in jQuery 2.1.4), we are encountering a
> DOMError 12 (Syntax Error) being generated in the
> WebCore::JsElementPrototypeFunctionMatches method.
> 
> +  377024 ( 408808 -  31784)    373 allocsBackTrace175CD6B0
> +     344 (    373 -     29)BackTrace175CD6B0allocations
> 
>     ntdll!RtlpCallInterceptRoutine+26
>     ntdll!RtlReAllocateHeap+3E7B1
>     WTF!realloc+43 (f:\dd\vctools\crt\crtw32\heap\realloc.c, 85)
>     WTF!WTF::fastRealloc+E (c:\bwa\wtf-7601.2.3\srcroot\wtf\fastmalloc.cpp,
> 155)
>     WTF!WTF::StringImpl::reallocateInternal<unsigned char>+72
> (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringimpl.cpp, 224)
>     WTF!WTF::StringImpl::reallocate+23
> (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringimpl.cpp, 231)
>     WTF!WTF::StringBuilder::reallocateBuffer<unsigned char>+4F
> (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringbuilder.cpp, 150)
>     WTF!WTF::StringBuilder::appendUninitializedSlow<unsigned char>+3C
> (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringbuilder.cpp, 235)
>     WTF!WTF::StringBuilder::append+73
> (c:\bwa\wtf-7601.2.3\srcroot\wtf\text\stringbuilder.cpp, 283)
>     JavaScriptCore!JSC::Interpreter::stackTraceAsString+C5
> (c:\bwa\javascriptcore-7601.2.3\srcroot\interpreter\interpreter.cpp, 580)
>     JavaScriptCore!JSC::addErrorInfoAndGetBytecodeOffset+2D2
> (c:\bwa\javascriptcore-7601.2.3\srcroot\runtime\error.cpp, 179)
>     JavaScriptCore!JSC::addErrorInfo+39
> (c:\bwa\javascriptcore-7601.2.3\srcroot\runtime\error.cpp, 190)
>     WebKit!WebCore::createDOMException+273
>     WebKit!WebCore::setDOMException+4B
>     WebKit!WebCore::jsElementPrototypeFunctionMatches+12A
> 
> The jsElementPrototypeFunctionMatches implementation calls the
> Element::matches method, which eventually takes us to
> SelectorQueryCache::add:
> 
>     SelectorQuery* SelectorQueryCache::add(const String& selectors,
> Document& document, ExceptionCode& ec)
>     SelectorQuery* Document::selectorQueryForString(const String&
> selectorString, ExceptionCode& ec)
>     bool Element::matches(const String& selector, ExceptionCode& ec)
> 
> When SelectorQueryCache::add parses the document looking for selectors, and
> does not find a match the exception code is set to 12 (SYNTAX_ERR) and it
> does an early return.

This statement is incorrect. SelectorQueryCache::add() parses the selector string that is passed. This has nothing to do with the Document, or matching elements. If SelectorQueryCache::add() throws, this means that parseSelector() returned an empty list of selector (because parsing the selector failed) or because selectorList.selectorsNeedNamespaceResolution() returns true.

[Chris Dumez]

All major browsers (Tested Safari, Firefox and Chrome) throw when calling:
document.body.matches(":visible")

They all complain about the selector being invalid.

[Chris Dumez]

(In reply to comment #5)
> All major browsers (Tested Safari, Firefox and Chrome) throw when calling:
> document.body.matches(":visible")
> 
> They all complain about the selector being invalid.

Apparently, ":visible" is a jQuery extension and not an actual CSS selector so it makes sense that Element.matches(":visible") throws. I don't know what jQuery's Element.is(":visible") does under the scene and which selector is actually being passed to WebKit.

[Brent Fulgham]

(In reply to comment #5)
> All major browsers (Tested Safari, Firefox and Chrome) throw when calling:
> document.body.matches(":visible")
> 
> They all complain about the selector being invalid.

Yes -- I see that now. It looks like jQuery catches the exception internally and discards it.

That means that the blob of these exception objects on the heap is expected, and GC should clean them up when it decides it needs the memory.

I'm closing this as works properly.

[Brent Fulgham]

(In reply to comment #6)
> (In reply to comment #5)
> > All major browsers (Tested Safari, Firefox and Chrome) throw when calling:
> > document.body.matches(":visible")
> > 
> > They all complain about the selector being invalid.
> 
> Apparently, ":visible" is a jQuery extension and not an actual CSS selector
> so it makes sense that Element.matches(":visible") throws. I don't know what
> jQuery's Element.is(":visible") does under the scene and which selector is
> actually being passed to WebKit.

I put a breakpoint in the element.matches code. When jQuery calls is(":visible"), that's exactly what we get. So, we attempt to match the invalid selector ":visible", and a syntax error is thrown.

This seems like it's just bad coding in jQuery. We do what the specification requires.