It is possible for unique anonymous functions to hash to the same value, thus showing up as one function in the profiler

• Component: Web Inspector (Deprecated)
• Status: RESOLVED DUPLICATE

[Francisco Tolmasky]

I am attaching a test case

[Francisco Tolmasky]

Created attachment 29456 [details]
unique anonymous functions

You need to run this from WebKit/LayoutTests/fast/profiler/

[Francisco Tolmasky]

This happens because functions are considered to be the "same" if they have the same name, line number, and url, which can happen for generated functions:

function create_function()
{
return function() { } // this function is unique, but has the same url, line number, and name every time it is created
}

[Francisco Tolmasky]

Created attachment 29461 [details]
Added reference to actual function to tell them apart.

[Oliver Hunt]

Comment on attachment 29461 [details]
Added reference to actual function to tell them apart.

The CallIdentifier should just take a JSObject* rather than the jsvalueptr

[Francisco Tolmasky]

Created attachment 29473 [details]
Changed JSValuePtr to JSObject*

[Francisco Tolmasky]

Created attachment 29474 [details]
Changed JSValuePtr to JSObject*

[Darin Adler]

Comment on attachment 29474 [details]
Changed JSValuePtr to JSObject*

The function pointer should be JSFunction*, not JSObject*. We always want to use as specific a type as possible.

> +        ProtectedPtr<JSObject> m_function;

We frown on uses of ProtectedPtr, and adding a new one is truly unfortunate. Is there an easy way to eliminate this?

> -        CallIdentifier(const UString& name, const UString& url, int lineNumber)
> +        CallIdentifier(const UString& name, const UString& url, int lineNumber, JSObject * function = NULL)

WebKit project coding style for this is JSObject* function = 0.

But default arguments are confusing. Do we really need a default here?

>              return UString::Rep::computeHash(reinterpret_cast<char*>(hashCodes), sizeof(hashCodes));

This isn't new code, but it's very strange. The char* version of the computeHash function should only be used on strings.

> -            return value.m_name.isNull() && value.m_url.isNull() && value.m_lineNumber == std::numeric_limits<unsigned>::max();
> +            return value.m_name.isNull() && value.m_url.isNull() && value.m_lineNumber == std::numeric_limits<unsigned>::max() && value.m_function.get() == NULL;

WebKit coding style for this would be !value.m_function

There's no need to use == NULL or get().

> -    return CallIdentifier(name.isEmpty() ? AnonymousFunction : name, function->body()->sourceURL(), function->body()->lineNo());
> +    return CallIdentifier(name.isEmpty() ? AnonymousFunction : name, function->body()->sourceURL(), function->body()->lineNo(), asObject(function));

You don't need the asObject cast here. The asObject function is for use when downcasting, but this is upcasting and you don't need an explicit cast at all for that. But also this should be JSFunction* as I mentioned above.

Since the only goal here is to avoid aliasing, I think it's unfortunate that we are keeping the entire JSFunction object, and everything it owns, around as long as we keep the profile around. I think we might want to experiment with an alternative solution where we assign consecutive numbers to each new function and then use these unique IDs instead of actually storing a pointer to the JSFunction. But the downside of this is that it would make execution slightly slower since we'd have to store and bump this number in the JSFunction constructor. And it would make JSFunction objects bigger since we'd need a place to store the unique ID, which could make functions too large for our fixed-size-cell garbage collector. However, maybe it's not the function, but the function body, that we want to use for the unique identifier. In that case it's the FunctionBodyNode that could store the ID, and I think we could easily spare the room there. The next available unique number would be stored in JSGlobalData.

review- because of the multiple issues above

[Oliver Hunt]

(In reply to comment #7)
> (From update of attachment 29474 [details] [review])
> The function pointer should be JSFunction*, not JSObject*. We always want to
> use as specific a type as possible.
> 
> > +        ProtectedPtr<JSObject> m_function;
It cannot be a JSFunction because being callable does not imply being a JSFunction

[Francisco Tolmasky]

Created attachment 29491 [details]
Style changes

Made described style adjustments, kept it as JSObject * per olli's note that the object may not be a JSFunction

[Darin Adler]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 29474 [details] [review] [review])
> > The function pointer should be JSFunction*, not JSObject*. We always want to
> > use as specific a type as possible.
> > 
> > > +        ProtectedPtr<JSObject> m_function;
> It cannot be a JSFunction because being callable does not imply being a
> JSFunction

I know that being callable does not imply JSFunction, but all the call sites in this patch *do* have a JSFunction.

[Darin Adler]

I don't think that a ProtectedPtr<JSObject> is the right solution here. I think we want to track unique function bodies, not unique JavaScript objects being called. This should be a RefPtr<FunctionBodyNode>, not a ProtectedPtr<JSObject>, or better could use a function body ID as I suggested in my earlier comment.

[Eric Seidel (no email)]

Comment on attachment 29491 [details]
Style changes

Neat to see Francisco contributing to WebKit!

Since there has been no response in the bug in over a month, r- based on Darin's above comment.

[Timothy Hatcher]

Looks like a dupe of bug 20155.

*** This bug has been marked as a duplicate of bug 20155 ***