Bug 26623 – [Chromium] Upstream V8Proxy

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

See summary.

[Nate Chapin]

Created attachment 31673 [details]
patch

[David Levin]

Comment on attachment 31673 [details]
patch

Thanks Nate for taking this on.  I know it is huge.  However, I think this needs to be scrubbed a bit more before an in depth review.  

Here's some steps to do to get this in better shape:

1. s/NULL/0/
2. Find all "!= 0" and "== 0" and get rid of them, replacing with appropriate logic.
3. Find variables_like_this and replace with variablesLikeThis.
4. getRidOfAbbrInVarNmes
5. Get rid of USE_VAR and replace with UNUSED_PARAM(whateverVariable); (from <wtf/UnusedParam.h>).
6. Change static locals in functions to use DEFINE_STATIC_LOCAL from wtf/StdLibExtras.h
7. Variable names should be descriptive words and "a" doesn't count :)  (Loop variables and iterators are the rare exception.)
8. End of line comments should only have one space before them.
9. Fix indentation in V8Proxy::SVGObjectWithContextToV8Object (actually there are several other places with 2 space indenting, scan the file and they will pop out at you).
10. Remove comments like "// static"
11. Get rid of variable names in function definitions when they add no information.  For example: "void AddToPage(Page* page) const;" but there are others.
12. Change MethodNames to be methodNames.
13. Spaces around |
14. Replace ASSERT(false); with ASSERT_NOT_REACHED() (from wtf/Assertions.h).
15. At about line 3128, the function bracing is at the end of line.  For example "void V8Proxy::CreateUtilityContext() {"
16. TODO(*): -> FIXME:

[Nate Chapin]

Created attachment 31805 [details]
patch2

The change in function names has grow the size of the CL substantial, I apologize in advance.

[Nate Chapin]

Created attachment 31806 [details]
patch3

Fixed the broken patching of V8Proxy.cpp into my webkit client.  Also, I hadn't applied all the latest change to the v8_proxy.cpp in src.chromium.org.

[Nate Chapin]

Created attachment 31815 [details]
patch4

[David Levin]

Comment on attachment 31815 [details]
patch4

I think we can finish with just one more round.  A few specific things to clean up below.



> Index: WebCore/ChangeLog
> [phone]  Nate Chapin  <[e-mail]>

I think this changelog need to be updated due to the new changes.


> Index: WebCore/bindings/v8/V8Proxy.h

very minor nit: General comment, WebKit uses one space after periods in comments. If you do a search and replace of ".  " with ". ", that would fix this up.

> +    class CSSStyleDeclaration;
> +    class ClientRectList;
> +    class DOMImplementation;
> +    class Element;
> +    class Event;
> +    class EventListener;
> +    class Frame;
> +    class HTMLCollection;
> +    class HTMLOptionsCollection;
> +    class HTMLElement;
> +    class HTMLDocument;
> +    class MediaList;
> +    class NamedNodeMap;
> +    class Node;
> +    class NodeList;
> +    class Screen;
> +    class String;
> +    class StyleSheet;
> +    class SVGElement;
> +    class DOMWindow;
> +    class Document;
> +    class EventTarget;
> +    class Event;
> +    class EventListener;
> +    class Navigator;
> +    class MimeType;
> +    class MimeTypeArray;
> +    class Plugin;
> +    class PluginArray;
> +    class StyleSheetList;
> +    class CSSValue;
> +    class CSSRule;
> +    class CSSRuleList;
> +    class CSSValueList;
> +    class NodeFilter;
> +    class ScriptExecutionContext;

These should be sorted (to avoid duplicates) for example Event is duplicated and a few others.


> + #if ENABLE(SVG)
> +    class SVGElementInstance;
> + #endif

Extra space before #'s here.

> +    class V8EventListener;
> +    class V8ObjectEventListener;

Put with the rest of the forward declarations.

> +    void logInfo(Frame* frame, const String& msg, const String& url);

Remove param name "frame".
s/msg/message/

> +    class GlobalHandleInfo {
> +    public:
> +        GlobalHandleInfo(void* host, GlobalHandleType type) : host_(host), type_(type) { }
> +        void* host_;

Use m_host

> +        GlobalHandleType type_;
Use m_type

> +    };
> +
> +#endif  // NDEBUG

single space before end of line comments.


> +         enum ErrorType {
> +             RANGE_ERROR,
> +             REFERENCE_ERROR,
> +             SYNTAX_ERROR,
> +             TYPE_ERROR,
> +             GENERAL_ERROR
> +         };

"Enum members should user InterCaps with an initial capital letter."
RangeError, etc.


> +        void setEventHandlerLineNumber(int lineNumber) { m_handlerLineno = lineNumber; }
> +        void finishedWithEvent(Event* event) { }

Remove "event".


> +        // Create a V8 wrapper for a C pointer
> +        static v8::Handle<v8::Value> wrapCPointer(void* cptr)
> +        {
> +            // Represent void* as int
> +            int addr = reinterpret_cast<int>(cptr);
> +            ASSERT(!(addr & 0x01));  // the address must be aligned.
Single space before end of line comment.

"T"he address...


> +        // Take C pointer out of a v8 wrapper

Add "."

> +#ifndef NDEBUG
> +        // Checks if a v8 value can be a DOM wrapper

Add "."


> +        template<typename T>
> +        static v8::Handle<v8::Value> convertToV8Object(V8ClassIndex::V8WrapperType type, PassRefPtr<T> imp)
> +        {
> +          return convertToV8Object(type, imp.get());

Fix indentation.

> +        // Wrap and unwrap JS event listeners
Add "."

> +        // Wrap JS node filter in C++

Add "."

> +#ifndef NDEBUG
> +        // For debugging and leak detection purpose
Add "."


> +        // Check whether a V8 value is a DOM Event wrapper
Add "."

> +        // Take C pointer out of a v8 wrapper
Add "."

> +        // The first parameter, desc_type, specifies the function descriptor
> +        // used to create JS object. The second parameter, cptr_type, specifies
> +        // the type of third parameter, impl, for type casting.
> +        // For example, a HTML element has HTMLELEMENT desc_type, but always
> +        // use NODE as cptr_type. JS wrapper stores cptr_type and impl as
> +        // internal fields.
> +        static v8::Local<v8::Object> instantiateV8Object(V8ClassIndex::V8WrapperType, V8ClassIndex::V8WrapperType, void*);

Rework the comment to remove parameters names since they aren't in the decl anymore.

> +        static v8::Local<v8::Context> utilityContext()
> +        {
> +          if (m_utilityContext.IsEmpty())
> +              createUtilityContext();
> +          return v8::Local<v8::Context>::New(m_utilityContext);
> +        }
Indentation is off.


> +    template <int tag, typename T>
> +    v8::Handle<v8::Value> V8Proxy::constructDOMObject(const v8::Arguments& args)
> +    {
> +        if (!args.IsConstructCall()) {
> +          V8Proxy::throwError(V8Proxy::TYPE_ERROR, "DOM object constructor cannot be called as a function.");
> +          return v8::Undefined();
Fix indentation and switch to the new "throwError".



> Index: WebCore/bindings/v8/V8Proxy.cpp

> +#include <algorithm>
> +#include <utility>
> +#include <v8.h>
> +#include <v8-debug.h>
> +#include <wtf/Assertions.h>
> +#include <wtf/StdLibExtras.h>
> +#include <wtf/UnusedParam.h>

Typically these come after the "" includes.


> +// Static utility context.

Seems obvious I'd remove the comment.

> +v8::Persistent<v8::Context> V8Proxy::m_utilityContext;

> +void batchConfigureAttributes(v8::Handle<v8::ObjectTemplate> instance, v8::Handle<v8::ObjectTemplate> proto, const BatchedAttribute* attributes, size_t attributeCount)
> +void batchConfigureConstants(v8::Handle<v8::FunctionTemplate> functionDescriptor, v8::Handle<v8::ObjectTemplate> proto, const BatchedConstant* constants, size_t constantCount)

If these functions are only used within this file, make them "static".


> +v8::Handle<v8::Value> V8Proxy::convertSVGObjectWithContextToV8Object(V8ClassIndex::V8WrapperType type, void* object)
> +{
> +    if (!object)
> +        return v8::Null();
> +
> +    v8::Persistent<v8::Object> result = getDOMSVGObjectWithContextMap().get(object);
> +    if (!result.IsEmpty())
> +        return result;
> +
> +    // Special case: SVGPathSegs need to be downcast to their real type
> +    if (type == V8ClassIndex::SVGPATHSEG)
> +        type = V8Custom::DowncastSVGPathSeg(object);
> +
> +    v8::Local<v8::Object> v8Object = instantiateV8Object(type, type, object);
> +    if (!v8Object.IsEmpty()) {
> +      result = v8::Persistent<v8::Object>::New(v8Object);
Fix indentation starting here.



> +    for (size_t i = 0; i < grouper.size(); ) {
> +        // Seek to the next key (or the end of the list).
> +        size_t nextKeyIndex = grouper.size();
> +        for (size_t j = i; j < grouper.size(); ++j) {
> +            if (grouper[i].first != grouper[j].first) {
> +              nextKeyIndex = j;
> +              break;
Fix indentation.



> +typedef HashMap<int, v8::FunctionTemplate*> FunctionTemplateMap;
> +
> +bool AllowAllocation::m_current = false;
> +
> +// JavaScriptConsoleMessages encapsulate everything needed to
> +// log messages originating from JavaScript to the Chrome console.
> +class JavaScriptConsoleMessage {
> +public:
> +    JavaScriptConsoleMessage(const String& string, const String& sourceID, unsigned lineNumber)
> +        : m_string(string)
> +        , m_sourceID(sourceID)
> +        , m_lineNumber(lineNumber) { }
Put the { } on seperate lines below.


> +static void handleConsoleMessage(v8::Handle<v8::Message> message, v8::Handle<v8::Value> data)
> +{
...
> +    bool useURL = (resourceName.IsEmpty() || !resourceName->IsString());
Remove unnecessary parens.

> +    String resourceNameString = (useURL) ? frame->document()->url() : ToWebCoreString(resourceName);

Remove unnecessary parens.

> +enum DelayReporting {
> +    REPORT_LATER,
> +    REPORT_NOW
> +};

"Enum members should user InterCaps with an initial capital letter."
  ReportLater
  ReportNow


> +
> +static void reportUnsafeAccessTo(Frame* target, DelayReporting delay)
> +{
> +    ASSERT(target);
> +    Document* targetDocument = target->document();
> +    if (!targetDocument)
> +        return;
> +
> +    Frame* source = V8Proxy::retrieveFrameForEnteredContext();
> +    if (!source || !source->document())
> +        return;  // Ignore error if the source document is gone.
Single space before end of line comment.

> +    String str = String::format("Unsafe JavaScript attempt to access frame "
> +        "with URL %s from frame with URL %s. "
> +        "Domains, protocols and ports must match.\n",
> +        targetDocument->url().string().utf8().data(),
> +        sourceDocument->url().string().utf8().data());

When indenting within a parenthesized expression, indent to the (.

> +
> +    // Build a console message with fake source ID and line number.
> +    const String kSourceID = "";
> +    const int kLineNumber = 1;
> +    JavaScriptConsoleMessage message(str, kSourceID, kLineNumber);
> +
> +    if (delay == REPORT_NOW) {
> +        // NOTE(tc): Apple prints the message in the target page, but it seems like

I think (tc) is an alias, so we can remove that, and it should probably say "Safari" instead of Apple.



> +bool V8Proxy::handleOutOfMemory()
> +{
...
> +    if (proxy) {
> +        // Clean m_context, and event handlers.
> +        proxy->clearForClose();
> +        // Destroy the global object.
> +        proxy->destroyGlobal();

"Destroy the global object." seems too obvious to have as a comment but leave a blank line to separate from the other commented item.


> +        // Isolate exceptions that occur when executing the code.  These
> +        // exceptions should not interfere with javascript code we might
> +        // evaluate from C++ when returning from here
Add "."

> +
> +        // Evaluating the JavaScript could cause the frame to be deallocated,
> +        // so we start the keep alive timer here.
> +        // Frame::keepAlive method adds the ref count of the frame and sets a
> +        // timer to decrease the ref count. It assumes that the current JavaScript
> +        // execution finishs before firing the timer.
s/finishs/finishes/

> +        // See issue 1218756 and 914430.
s/issue/issues/  These are internal bug numbers so it is odd to keep them here but maybe they are good for reference.  Ask around and see what people think should be done with this.

> +v8::Local<v8::Value> V8Proxy::callFunction(v8::Handle<v8::Function> function, v8::Handle<v8::Object> receiver, int argc, v8::Handle<v8::Value> args[])
> +{
...
> +        // Evaluating the JavaScript could cause the frame to be deallocated,
> +        // so we start the keep alive timer here.
> +        // Frame::keepAlive method adds the ref count of the frame and sets a
> +        // timer to decrease the ref count. It assumes that the current JavaScript
> +        // execution finishs before firing the timer.
> +        // See issue 1218756 and 914430.

Same code comment as before with the same issues.


> +v8::Local<v8::Value> V8Proxy::newInstance(v8::Handle<v8::Function> constructor, int argc, v8::Handle<v8::Value> args[])
> +{
...
> +        // See comment in V8Proxy::callFunction.

I'm happy the comment isn't here again, but maybe we can reduce the two copies to one using a technique like this :)


> +
> +v8::Persistent<v8::FunctionTemplate> V8Proxy::getTemplate(V8ClassIndex::V8WrapperType type)
> +{
> +    v8::Persistent<v8::FunctionTemplate>* cacheCell = V8ClassIndex::GetCache(type);
> +    if (!(*cacheCell).IsEmpty())

The more typical way to write this would be: 
    if (!cacheCell->IsEmpty())

> +
> +    // not found

How about "Not in the cache."


> +void V8Proxy::clearForNavigation()
> +{
> +    // disconnect all event listeners
> +    disconnectEventListeners();

Remove comment.

> +void V8Proxy::setSecurityToken()
> +{
> +    Document* document = m_frame->document();
> +    // Setup security origin and security token
Add "."


> +v8::Persistent<v8::Context> V8Proxy::createNewContext(v8::Handle<v8::Object> global)
> +{

> +    const char** extensionNames = new const char*[m_extensions.size()];

Make this a OwnArrayPtr (and get rid of the "delete[] extensionNames").

> +void V8Proxy::initContextIfNeeded()
> +{
...
> +    DEFINE_STATIC_LOCAL(bool, isV8Initialized, (false));

You really only need to use "DEFINE_STATIC_LOCAL" when you have a class (something with a destructor).  For bool's, int's etc., just define them like usual.
There may have been other instances of this, so just take this comment to apply to all of them.

> +
> +    // Store the first global object created so we can reuse it.
> +    if (m_global.IsEmpty()) {
> +        m_global = v8::Persistent<v8::Object>::New(v8Context->Global());
> +        // Bail out if allocation of the first global objects fails.
> +        if (m_global.IsEmpty()) {
> +          disposeContextHandles();
> +          return;

Fix indentation.

[Nate Chapin]

Created attachment 31893 [details]
patch5

batchConfigure{Constants,Attributes} are used outside of V8Proxy.cpp, so leaving them non-static.

Merged in another revision to v8_proxy.cpp downstream as well.

[David Levin]

Comment on attachment 31893 [details]
patch5

I think it has made huge progress and we should land it.

Please fix this *before* landing (just fix it and land it *without* uploading another patch):

  > WebCore/bindings/v8/V8DOMMap.cpp

   Has unresolved conflict in it at line 495.





Ideally we'd have a *subsequent* patch to fix the following few things:  (Please *don't* update this patch with these fixes.)
  
  > WebCore/bindings/v8/V8Proxy.cpp


  1. Finish getting rid of USE_VAR and replace with UNUSED_PARAM.
  2. In all classes, public: protected: private: shouldn't be indented.
  3. In class GrouperItem,
     a. the variable names need to have this m_style
     b. group_id should be groupId (for the method and variable)
     c. For the constructor: "Each member (and superclass) should be indented on a separate line"
  4. In several of the class the functions have braces at the end of line.
  5. In ObjectGrouperVisitor::visitDOMWrapper
     a. typo "elment"
     b. braces around single line statement
  6. ObjectGrouperVisitor::grouper_ naming.
  7. JavaScriptConsoleMessage::JavaScriptConsoleMessage constructor braces are too indented.
  8. ConsoleMessageManager::processDelayedMessages braces on single line statement.
  9. There are several places that do the two spaces before end of line comments (should be one) -- search for ";  //"
  10. V8Proxy::setContextDebugId has end of line brace.

[Dimitri Glazkov (Google)]

We're going to have to roll this one out, because we're having problems reconciling this downstream.

For big pieces of the puzzle like this:

1) Let's first land the code upstream, however ugly it is (look at V8Index!) and switch over to using it downstream.

2) Refactor for code style upstream.

I know this means we'll have non-conforming code in the WebKit tree, but it's temporary and sure beats struggling with keeping up with changes while refactoring, struggling with landing, then realizing we've missed one thing and having to revert both sides.

[Dimitri Glazkov (Google)]

We've talked and despite our better judgement, we're going to try to keep this patch and fix it as-is.