Bug 21918 – A JavaScript Exception abstraction

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Dimitri Glazkov (Google)]

To improve separation of concerns, an abstraction can be introduced to pass and query exception information in DOM manipulation code.

[Dimitri Glazkov (Google)]

Created attachment 24705 [details]
ExceptionContext patch

[Oliver Hunt]

Comment on attachment 24705 [details]
ExceptionContext patch

I really don't like this -- you're making inline functions no longer be inline, and are increasing the verbosity of a fairly standard concept (eg. methods that taken an execution context). In terms of code style ExceptionCaptcher class is only used in V8, yet is being included behind an ifdef in the "cross platform" part of the header.

At the very least ExceptionCapther should be in its own file, rather than creating new files which already have traces of ifdef hell.

[Dimitri Glazkov (Google)]

Created attachment 24718 [details]
Removed ExceptionCatcher, added inlines

You know, I woke up this morning and had exact same thought about ExceptionCatcher. Here's an updated patch, with it removed. I also added "inline" to all ExceptionContext methods, and hope that the compiler will agree with me that they are simple enough to be treated as such.

Let me know whatcha think.

[Sam Weinig]

Comment on attachment 24718 [details]
Removed ExceptionCatcher, added inlines

> +
> +#include <wtf/Noncopyable.h>
> +#include "ScriptController.h"
These should be sorted.

> +
> +typedef JSC::JSValue* JSException;
No JSException is being defined for V8 here.  I don't really see the point of this typedef since JSException is never used.

> +#if USE(V8)
> +class ExceptionCatcher;
What does an ExceptionCatcher do?  Why is necessary to store both it and the exception.

> +    ExceptionContext(Node*);
> +#if USE(V8)
> +    ExceptionContext();
How is this empty constructor used.  Perhaps a link to the corresponding Chromium code would help.

> +    bool hadException();
> +    JSException exception() const;
This method seems to be unused.
> +
> +    // Returns a non-exception code object.
> +    static JSException noException();
As does this one.

> +
> +private:
> +#if USE(V8)
> +    friend class ExceptionCatcher;
> +    void setException(JSException exception) { m_exception = exception; }
> +    void setExceptionCatcher(ExceptionCatcher*);
> +    JSException m_exception;
> +    ExceptionCatcher* m_exceptionCatcher;
> +#elif USE(JSC)
> +    JSC::ExecState* m_exec;
> +#endif

This is a lot of engine specific code.  Can this be abstracted better?

It also seems like this is something that should work with other bindings as well.  So that a Objective-C NodeFilter could store a thrown exception as well.

r-.

[Dimitri Glazkov (Google)]

Sam, thanks for the irc conversation -- I think I have a pretty good idea on how to make this simpler and less engine-dependent. Patch coming up.

[Dimitri Glazkov (Google)]

Created attachment 24847 [details]
Platform independence with magic headers

Re-worked the patch to get rid of #ifdefs by using magic headers. This can also now be done with one big #ifdef at the top of ExceptionContext.h, but I took the scenic route, because it allows for more flexibility for other platforms to incorporate exception-carrying goodness.

[Dimitri Glazkov (Google)]

Created attachment 24849 [details]
Platform independence with magic headers

Cleaned up log entry, reordered project file, added ExceptionContextData.h to WebCore.vcproj

[Dimitri Glazkov (Google)]

Comment on attachment 24847 [details]
Platform independence with magic headers

Oops, forgot to obsolete the patch

[Dimitri Glazkov (Google)]

Also, corresponding Chromium reviews:

http://codereview.chromium.org/9018
http://codereview.chromium.org/9019

[Darin Adler]

Comment on attachment 24849 [details]
Platform independence with magic headers

> --- /dev/null
> +++ b/WebCore/dom/ExceptionContext.cpp
> @@ -0,0 +1,39 @@
> +/*
> + * Copyright (C) 2008 Google Inc.
> + * Copyright (C) 2008 Apple Inc. All rights reserved.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + */
> +
> +#include "config.h"
> +#include "ExceptionContext.h"
> +
> +#include "JSDOMBinding.h"
> +#include "Node.h"
> +
> +namespace WebCore {
> +
> +ExceptionContext::ExceptionContext(Node* node)
> +    : m_data(execStateFromNode(node))
> +{
> +}
> +
> +bool ExceptionContext::hadException()
> +{
> +    return m_data->hadException();
> +}
> +
> +}  // namespace WebCore

I don't understand this source file. It's in the "dom" directory, but it has JavaScriptCore-specific JavaScript code in it, calling execStateFromNode. How does that work?

I think the name ExceptionContext is a little misleading. To be clear, ExecState* is not an "exception" context for JavaScriptCore. It's more than that. It's both the context needed to execute more JavaScript code, and also, almost incidentally, a way to return the JavaScript exception to the caller. If we only wanted to give a way to return the JavaScript exception, we could just use an out parameter with a place to put the exception, but that wouldn't suffice because JavaScript execution requires more context than that. The comments in this new ExceptionContext class seem to be vague on this point, and the class name too.

> +        ExceptionContext(Node* node);

It seems pretty mysterious what this constructor would do. I know, but only because I designed the old code, that this is going to figure out the correct JavaScript execution context given a DOM node. But if the class really was just an "exception context", then you wouldn't need this argument. I also don't think that providing this as a constructor for an exception context is good encapsulation; it's more about how the DOM uses JavaScript than it is about the JavaScript machinery itself.

> +        ExceptionContext(ExceptionContextData data) : m_data(data) {}

If ExecutionContext is going to be more than just a single ExceptionContextData, then I would think that it's a hidden implementation detail what ExceptionContextData is, and there shouldn't be a public constructor that takes one of them. If we're really going to make it public that ExceptionContextData is all that's inside an ExceptionContext, then I don't understand why we need both the ExceptionContext and ExceptionContextData types. Also if ExceptionContextData is any non-trivial object, I think we'd want to pass const ExceptionContextData&.

> +        ~ExceptionContext() {}

This definition of the constructor should be omitted. The compiler will do the same thing if you don't declare a constructor at all.

> +        ExceptionContextData data() const { return m_data; }
> +        void setData(ExceptionContextData data) { m_data = data; }

Same comments apply here as in the constructor. I see no value added by the ExceptionContext class if it's just a holder for a single ExceptionContextData. Seems over-engineered. A typedef for ExecState* would accomplish the same thing.

Unless there's future plan for putting more into ExceptionContext I suggest just using typedef ExecState ExceptionContext; adding another class here isn't adding additional value.

I'm going to say review- on this. I understand the need to make this code JavaScript-engine-independent, but I think it can be done either more simply than this, or in a more complete way.

A great patch would also make some progress on addressing the issue of propagating exceptions from languages other than JavaScript, such as Objective-C; but I guess I'm not asking for that since I don't really see how to solve that problem.

[Dimitri Glazkov (Google)]

Created attachment 25364 [details]
ScriptState (concept only)

I would like to get this right, so please bear with me.

It looks like for the purposes of carrying the exception information, we actually need only the non-CallFramey members of JSC::ExecState: the hadException(), exception() and friends. So, perhaps the right solution would be to introduce a ScriptState abstraction, which only exposes these methods?

See the attachment. There, I just typedef'd ScriptState, but potentially, it seems logical to have it as a separate class with a callFrame() method, which then gives you the actual CallFrame info, i.e.:

  Script State --(has a)--> Script CallFrame 

Does this make better sense? I plead for guidance and thought-sharing. I don't mind the hazing :)

Is the general direction that JSC::ExecState would eventually become JSC::CallFrame and at some point shed the globalData accessors?

In the context of Chromium, it would perfect to have a non JSC-specific class, which can be passed through WebCore to NodeFilter, NodeIterator, et al. and store exception information.

I don't know much about non-JS bindings, but would be happy to hear any ideas as well. It seems we need cover scenarios with Non-JS node filter, as well as JS-originated node filter, but non-JS iteration calls.

[Geoffrey Garen]

> ScriptState (concept only)

Concept seems reasonable to me.

Did you forget to attach ScriptState.h?

> Is the general direction that JSC::ExecState would eventually become
> JSC::CallFrame and at some point shed the globalData accessors?

ExecState is CallFrame today. Global data is accessed through the CallFrame.

[Dimitri Glazkov (Google)]

Created attachment 25490 [details]
Added ScriptState.h

Sorry, added ScriptState (see updated patch). I am currently trying to hack on MiniBrowser to introduce myself to the wondrous way of Objective-C in order to better understand how ScriptState could be used for carrying exception information across binding boundaries.

[Dimitri Glazkov (Google)]

Created attachment 25541 [details]
Script Patch v3 (with project changes)

I updated the patch with XCode and VC project changes. I think this is ready for review (no longer work-in-progress).

As the next step, I am working on making ScriptState more meaningful by introducing it into Obj-C bindings, drawing inspiration from eseidel's work on https://bugs.webkit.org/show_bug.cgi?id=18234.

But that's a whole 'nother story (and bug filed). 

BTW, if you know of any other bugs that deal with carrying exception information across binding boundaries, please let me know.

[Geoffrey Garen]

Comment on attachment 25541 [details]
Script Patch v3 (with project changes)

ScriptState.h is in attachment 25490 [details], but not attachment 25491 [details]. I'm ready to r+, but it could be confusing to land, so please provide a final patch that includes all the project changes and ScriptState.h

[Dimitri Glazkov (Google)]

Created attachment 25543 [details]
ScriptState v4 (with ScriptState.h!)

git FAIL. Apparently, there's a big difference between "git diff" and "git diff master"!

Proper patch attached.

[Dimitri Glazkov (Google)]

Created attachment 25544 [details]
ScriptState v4.1 (with ScriptState.h!)

There was a weird compatibility flag in the XCode project. Removed.

[Geoffrey Garen]

Comment on attachment 25544 [details]
ScriptState v4.1 (with ScriptState.h!)

r=me

[Eric Seidel (no email)]

project file changes didn't apply cleanly. will look at this again in a bit.  dglazkov needs teh commit bit.

[Dimitri Glazkov (Google)]

Created attachment 25747 [details]
ScriptState v5 (rebased to trunk)

Rebased to trunk for ease of landing. I am assuming since it's already r+'d, it's just a matter of landing, right?

[Eric Seidel (no email)]

(In reply to comment #20)
> Created an attachment (id=25747) [review]
> ScriptState v5 (rebased to trunk)
> 
> Rebased to trunk for ease of landing. I am assuming since it's already r+'d,
> it's just a matter of landing, right?

Yes, just needs landing.  I'll do that this afternoon.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/bindings/js/JSDOMBinding.cpp
	M	WebCore/bindings/js/JSDOMBinding.h
	M	WebCore/bindings/js/JSNodeFilterCondition.h
	M	WebCore/dom/NodeFilter.cpp
	M	WebCore/dom/NodeFilter.h
	M	WebCore/dom/NodeFilterCondition.cpp
	M	WebCore/dom/NodeFilterCondition.h
	M	WebCore/dom/NodeIterator.cpp
	M	WebCore/dom/NodeIterator.h
	M	WebCore/dom/Traversal.cpp
	M	WebCore/dom/Traversal.h
	M	WebCore/dom/TreeWalker.cpp
	M	WebCore/dom/TreeWalker.h
Committed r39003

[Darin Adler]

Comment on attachment 25747 [details]
ScriptState v5 (rebased to trunk)

Yes, that's right. No review needed for rebasing this. Next time you can just attach the newer patch with a comment -- you don't even *really* need to set the review flag.