Remove IDBNotFoundError ExceptionCode

Remove IDBNotFoundError ExceptionCode

Instead of passing an ExceptionCode (int) around to signify
an exception, we now pass an ExceptionState object. This is
so that we can pass more data out than just a single int.

The ExceptionState object has methods for throwing an exception with an optional error message.

There are also temporary converters so that functions can
still use ExceptionCode until the entire code base has
transitioned. Once that is done the ExcetionState code can
be simplified further.

BUG=252233
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153938

[arv (Not doing code reviews), 2013-07-01 22:29:30]

Feedback wanted

There is some room for performance improvements once all the error handling has
transitioned.

                      master          ExceptionState
dom-attr      runs/s  112.80 ± 0.28%  112.00 ± 0.48% 0.71% Worse
dom-modify    runs/s   39.84 ± 1.71%   39.38 ± 1.79%
dom-query     runs/s  303.37 ± 0.53%  322.29 ± 1.34% 6.24% Better
dom-traverse  runs/s   95.19 ± 0.34%   95.04 ± 0.36%

https://codereview.chromium.org/18398002/diff/2001/Source/bindings/v8/Excepti...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/18398002/diff/2001/Source/bindings/v8/Excepti...
Source/bindings/v8/ExceptionState.h:69: if (m_code) {
The idea is that all the uses would do throwDOMException and we don't need to
store the m_code and all we need to know is if there was an exception (some
places care).

https://codereview.chromium.org/18398002/diff/2001/Source/bindings/v8/Excepti...
Source/bindings/v8/ExceptionState.h:92: ExceptionCode m_code;
During the transition we need both m_code and m_exceptionThrown. Once all the
code has transitioned to ExceptionState we only need a single boolean flag.

https://codereview.chromium.org/18398002/diff/2001/Source/core/dom/DOMExcepti...
File Source/core/dom/DOMException.cpp (right):

https://codereview.chromium.org/18398002/diff/2001/Source/core/dom/DOMExcepti...
Source/core/dom/DOMException.cpp:41: { "IndexSizeError", "Index or size was
negative, or greater than the allowed value.", 1 },
I'm tempted to use an .in file for this table and generate something better than
this table.

https://codereview.chromium.org/18398002/diff/2001/Source/core/editing/Replac...
File Source/core/editing/ReplaceNodeWithSpanCommand.cpp (left):

https://codereview.chromium.org/18398002/diff/2001/Source/core/editing/Replac...
Source/core/editing/ReplaceNodeWithSpanCommand.cpp:65:
parentNode->removeChild(nodeToReplace, ASSERT_NO_EXCEPTION);
These all default to ASSERT_NO_EXCEPTION.

https://codereview.chromium.org/18398002/diff/2001/Source/core/inspector/Insp...
File Source/core/inspector/InspectorIndexedDBAgent.cpp (right):

https://codereview.chromium.org/18398002/diff/2001/Source/core/inspector/Insp...
Source/core/inspector/InspectorIndexedDBAgent.cpp:213: IgnorableExceptionState
es;
Maybe another subclass... ignore is not really the right thing here. We do care
if there was an error but it would never throw. How about
NonThrowExceptionState?

https://codereview.chromium.org/18398002/diff/2001/Source/modules/indexeddb/I...
File Source/modules/indexeddb/IDBDatabase.cpp (right):

https://codereview.chromium.org/18398002/diff/2001/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBDatabase.cpp:228: // FIXME: Should use constant
Where would be a good place to put this string?

[haraken, 2013-07-01 23:55:04]

Where does the performance gain come from? It looks like there is no change in
the call path of dom-query.

https://codereview.chromium.org/18398002/diff/2001/Source/bindings/v8/Excepti...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/18398002/diff/2001/Source/bindings/v8/Excepti...
Source/bindings/v8/ExceptionState.h:50: virtual void throwDOMException(const
ExceptionCode& ec,  const char* message = 0)

You might want to put the method in a cpp file, since exceptions wouldn't be
performance-sensitive and these methods will be used in a lot of places.

[arv (Not doing code reviews), 2013-07-02 00:34:18]

On 2013/07/01 23:55:04, haraken wrote:
> Where does the performance gain come from? It looks like there is no change in
> the call path of dom-query.

This is something that is confusing me too. I've run these tests tens of times
(using the instructions Jochen gave me) and it is consistent.

>
https://codereview.chromium.org/18398002/diff/2001/Source/bindings/v8/Excepti...
> File Source/bindings/v8/ExceptionState.h (right):
> 
>
https://codereview.chromium.org/18398002/diff/2001/Source/bindings/v8/Excepti...
> Source/bindings/v8/ExceptionState.h:50: virtual void throwDOMException(const
> ExceptionCode& ec,  const char* message = 0)
> 
> You might want to put the method in a cpp file, since exceptions wouldn't be
> performance-sensitive and these methods will be used in a lot of places.

Will do.

[jsbell, 2013-07-02 18:48:08]

https://codereview.chromium.org/18398002/diff/2001/Source/core/dom/DOMExcepti...
File Source/core/dom/DOMException.h (right):

https://codereview.chromium.org/18398002/diff/2001/Source/core/dom/DOMExcepti...
Source/core/dom/DOMException.h:42: static PassRefPtr<DOMException>
create(ExceptionCode, const char* = 0);
nit: name the argument.

https://codereview.chromium.org/18398002/diff/2001/Source/core/inspector/Insp...
File Source/core/inspector/InspectorIndexedDBAgent.cpp (right):

https://codereview.chromium.org/18398002/diff/2001/Source/core/inspector/Insp...
Source/core/inspector/InspectorIndexedDBAgent.cpp:213: IgnorableExceptionState
es;
On 2013/07/01 22:29:31, arv wrote:
> Maybe another subclass... ignore is not really the right thing here. We do
care
> if there was an error but it would never throw. How about
> NonThrowExceptionState?

Agreed, and I don't have a better suggestion.

https://codereview.chromium.org/18398002/diff/2001/Source/modules/indexeddb/I...
File Source/modules/indexeddb/IDBDatabase.cpp (right):

https://codereview.chromium.org/18398002/diff/2001/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBDatabase.cpp:228: // FIXME: Should use constant
On 2013/07/01 22:29:31, arv wrote:
> Where would be a good place to put this string?

For DOMError messages minted in Blink and Chromium, we just have the strings
inline or static const char* at the top of the file... this is used in 6 places,
3 files. The messages could be made more specific: 4 sites (2 files) could use
"requested object store '%s'" and 2 sites (1 file) would use "requested index
'%s'". That'd mean if they were at the top of the file that in the worst case
the string would be repeated twice.

But if we want to use shared constants, we could also add IndexedDB.cpp.

https://codereview.chromium.org/18398002/diff/2001/Source/modules/indexeddb/I...
File Source/modules/indexeddb/IDBTransaction.cpp (right):

https://codereview.chromium.org/18398002/diff/2001/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBTransaction.cpp:337: IndexedDB::TransactionMode
IDBTransaction::stringToMode(const String& modeString, ExceptionCode& ec)
Still needs updating, obviously. (Hence the DO NOT SUBMIT)

https://codereview.chromium.org/18398002/diff/2001/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBTransaction.cpp:345: ec = TypeError;
This will become es.throwTypeError(), but es.throwDOMException(TypeError) would
work for now?

[arv (Not doing code reviews), 2013-07-09 19:12:57]

I ran the perf tests on my MacPro as well and the outcome is similar. dom-query
is faster with my patch?!?!

[arv (Not doing code reviews), 2013-07-09 23:41:03]

I believe this is ready for review. PTAL.

https://codereview.chromium.org/18398002/diff/2001/Source/bindings/v8/Excepti...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/18398002/diff/2001/Source/bindings/v8/Excepti...
Source/bindings/v8/ExceptionState.h:50: virtual void throwDOMException(const
ExceptionCode& ec,  const char* message = 0)
On 2013/07/01 23:55:05, haraken wrote:
> 
> You might want to put the method in a cpp file, since exceptions wouldn't be
> performance-sensitive and these methods will be used in a lot of places.

Done.

https://codereview.chromium.org/18398002/diff/2001/Source/core/dom/DOMExcepti...
File Source/core/dom/DOMException.h (right):

https://codereview.chromium.org/18398002/diff/2001/Source/core/dom/DOMExcepti...
Source/core/dom/DOMException.h:42: static PassRefPtr<DOMException>
create(ExceptionCode, const char* = 0);
On 2013/07/02 18:48:08, jsbell wrote:
> nit: name the argument.

Done.

https://codereview.chromium.org/18398002/diff/2001/Source/core/inspector/Insp...
File Source/core/inspector/InspectorIndexedDBAgent.cpp (right):

https://codereview.chromium.org/18398002/diff/2001/Source/core/inspector/Insp...
Source/core/inspector/InspectorIndexedDBAgent.cpp:213: IgnorableExceptionState
es;
On 2013/07/02 18:48:08, jsbell wrote:
> On 2013/07/01 22:29:31, arv wrote:
> > Maybe another subclass... ignore is not really the right thing here. We do
> care
> > if there was an error but it would never throw. How about
> > NonThrowExceptionState?
> 
> Agreed, and I don't have a better suggestion.

Done.

https://codereview.chromium.org/18398002/diff/2001/Source/modules/indexeddb/I...
File Source/modules/indexeddb/IDBDatabase.cpp (right):

https://codereview.chromium.org/18398002/diff/2001/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBDatabase.cpp:228: // FIXME: Should use constant
On 2013/07/02 18:48:08, jsbell wrote:
> On 2013/07/01 22:29:31, arv wrote:
> > Where would be a good place to put this string?
> 
> For DOMError messages minted in Blink and Chromium, we just have the strings
> inline or static const char* at the top of the file... this is used in 6
places,
> 3 files. The messages could be made more specific: 4 sites (2 files) could use
> "requested object store '%s'" and 2 sites (1 file) would use "requested index
> '%s'". That'd mean if they were at the top of the file that in the worst case
> the string would be repeated twice.
> 
> But if we want to use shared constants, we could also add IndexedDB.cpp.

Went with a single constant for now.

[jsbell, 2013-07-10 00:41:31]

IDB changes LGTM

https://codereview.chromium.org/18398002/diff/30001/Source/bindings/v8/Except...
File Source/bindings/v8/ExceptionState.cpp (right):

https://codereview.chromium.org/18398002/diff/30001/Source/bindings/v8/Except...
Source/bindings/v8/ExceptionState.cpp:41: if (m_exceptionThrown)
Out of curiosity, do we ever hit this case? Perhaps in the future we should
ASSERT here, as it would mean some algorithm is not terminating after throwing
when it should.

(There's probably somewhere in the platform that does the equivalent of an "if
those steps threw, throw XYZ instead" which perhaps should be made explicit.)

Anyway, not for this patch.

https://codereview.chromium.org/18398002/diff/30002/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/18398002/diff/30002/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:164: // Setting an explicit
ExceptionState here would defer handling the already thrown exception.
This comment should probably be reworded. And possibly update the FIXME above.

[haraken, 2013-07-10 00:43:36]

LGTM for bindings/ and core/

[arv (Not doing code reviews), 2013-07-10 02:12:58]

https://codereview.chromium.org/18398002/diff/30001/Source/bindings/v8/Except...
File Source/bindings/v8/ExceptionState.cpp (right):

https://codereview.chromium.org/18398002/diff/30001/Source/bindings/v8/Except...
Source/bindings/v8/ExceptionState.cpp:41: if (m_exceptionThrown)
On 2013/07/10 00:41:31, jsbell wrote:
> Out of curiosity, do we ever hit this case? Perhaps in the future we should
> ASSERT here, as it would mean some algorithm is not terminating after throwing
> when it should.
> 
> (There's probably somewhere in the platform that does the equivalent of an "if
> those steps threw, throw XYZ instead" which perhaps should be made explicit.)
> 
> Anyway, not for this patch.

An assert would be good. I'll add that.

The end goal is to not have to keep track of m_exceptionThrown or m_code.

https://codereview.chromium.org/18398002/diff/30002/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/18398002/diff/30002/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:164: // Setting an explicit
ExceptionState here would defer handling the already thrown exception.
On 2013/07/10 00:41:32, jsbell wrote:
> This comment should probably be reworded. And possibly update the FIXME above.

ScriptState keeps the last exception but only for exceptions thrown using
scriptState. This definitely needs some work.

One idea I had was to make ScriptState extend ExceptionState. There is currently
some overlap.

I'll update the comments.

[Mike West, 2013-07-10 09:40:30]

This is great, Erik. Thank you for working on it.

https://chromiumcodereview.appspot.com/18398002/diff/30002/Source/bindings/v8...
File Source/bindings/v8/custom/V8NodeCustom.cpp (right):

https://chromiumcodereview.appspot.com/18398002/diff/30002/Source/bindings/v8...
Source/bindings/v8/custom/V8NodeCustom.cpp:69: ExceptionState
es(args.GetIsolate());
This all looks perfectly reasonable, but doesn't seem related to the CL's stated
purpose (IDB changes). Would you mind splitting the Node changes out into a
separate patch (or updating the description with relevant detail)?

https://chromiumcodereview.appspot.com/18398002/diff/30002/Source/core/dom/DO...
File Source/core/dom/DOMException.h (right):

https://chromiumcodereview.appspot.com/18398002/diff/30002/Source/core/dom/DO...
Source/core/dom/DOMException.h:55: explicit DOMException(unsigned short m_code,
const char* name, const char* message);
You can drop 'explicit'.

https://chromiumcodereview.appspot.com/18398002/diff/30002/Source/core/editin...
File Source/core/editing/ReplaceNodeWithSpanCommand.cpp (right):

https://chromiumcodereview.appspot.com/18398002/diff/30002/Source/core/editin...
Source/core/editing/ReplaceNodeWithSpanCommand.cpp:65:
parentNode->removeChild(nodeToReplace);
Why did you drop the ASSERT_NO_EXCEPTION here and in the other few 'removeChild'
calls? That doesn't seem to be what this CL is actually doing... Leftover from
previous experiments?

https://chromiumcodereview.appspot.com/18398002/diff/30002/Source/core/html/H...
File Source/core/html/HTMLTableElement.h (right):

https://chromiumcodereview.appspot.com/18398002/diff/30002/Source/core/html/H...
Source/core/html/HTMLTableElement.h:38: typedef int ExceptionCode;
This doesn't seem related to this CL. What changed that made the typedef
necessary?

[arv (Not doing code reviews), 2013-07-10 15:37:23]

https://codereview.chromium.org/18398002/diff/30002/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8NodeCustom.cpp (right):

https://codereview.chromium.org/18398002/diff/30002/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8NodeCustom.cpp:69: ExceptionState
es(args.GetIsolate());
On 2013/07/10 09:40:30, Mike West wrote:
> This all looks perfectly reasonable, but doesn't seem related to the CL's
stated
> purpose (IDB changes). Would you mind splitting the Node changes out into a
> separate patch (or updating the description with relevant detail)?

Done.

https://codereview.chromium.org/18398002/diff/30002/Source/core/dom/DOMExcept...
File Source/core/dom/DOMException.h (right):

https://codereview.chromium.org/18398002/diff/30002/Source/core/dom/DOMExcept...
Source/core/dom/DOMException.h:55: explicit DOMException(unsigned short m_code,
const char* name, const char* message);
On 2013/07/10 09:40:30, Mike West wrote:
> You can drop 'explicit'.

Done.

https://codereview.chromium.org/18398002/diff/30002/Source/core/editing/Repla...
File Source/core/editing/ReplaceNodeWithSpanCommand.cpp (right):

https://codereview.chromium.org/18398002/diff/30002/Source/core/editing/Repla...
Source/core/editing/ReplaceNodeWithSpanCommand.cpp:65:
parentNode->removeChild(nodeToReplace);
On 2013/07/10 09:40:30, Mike West wrote:
> Why did you drop the ASSERT_NO_EXCEPTION here and in the other few
'removeChild'
> calls? That doesn't seem to be what this CL is actually doing... Leftover from
> previous experiments?

These are useless since it defaults to ASSERT_NO_EXCEPTION anyway.

I'll extract them to a different cl.

https://codereview.chromium.org/18398002/diff/30002/Source/core/html/HTMLTabl...
File Source/core/html/HTMLTableElement.h (right):

https://codereview.chromium.org/18398002/diff/30002/Source/core/html/HTMLTabl...
Source/core/html/HTMLTableElement.h:38: typedef int ExceptionCode;
On 2013/07/10 09:40:30, Mike West wrote:
> This doesn't seem related to this CL. What changed that made the typedef
> necessary?

Not necessary, just a cleanup. I'll remove for now.

[commit-bot: I haz the power, 2013-07-10 16:01:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arv@chromium.org/18398002/43001

[commit-bot: I haz the power, 2013-07-10 16:12:48]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[arv (Not doing code reviews), 2013-07-10 16:25:24]

Missing LGTM from an OWNER for these files:
    public/platform/WebIDBDatabaseException.h

[dglazkov, 2013-07-10 17:49:12]

LGTM.

[commit-bot: I haz the power, 2013-07-10 17:50:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arv@chromium.org/18398002/43001

[commit-bot: I haz the power, 2013-07-10 18:38:01]

Retried try job too often on win_layout_rel for step(s) webkit_lint,
webkit_python_tests, webkit_tests, webkit_unit_tests, weborigin_unittests,
wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-07-10 19:09:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arv@chromium.org/18398002/43001

[commit-bot: I haz the power, 2013-07-10 19:43:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-10 20:08:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arv@chromium.org/18398002/64001

[michaeln, 2013-07-10 20:31:01]

https://codereview.chromium.org/18398002/diff/64001/Source/bindings/v8/Except...
File Source/bindings/v8/ExceptionState.cpp (right):

https://codereview.chromium.org/18398002/diff/64001/Source/bindings/v8/Except...
Source/bindings/v8/ExceptionState.cpp:43: V8ThrowException::setDOMException(ec,
message, m_isolate);
Does this call invoke script?

If so, i think this may change the control flow in ways that might not be good.
The old way, script wasn't invoked until the c++ stack unwound up to very near
when v8 crossed over to c++. The thing is re-entrancy. Once script is running it
can then call back into c++. In the new way, we'll be reentering c++ prior to
having unwound.

There might be some stack overflow opportunities here?

Also, assuming there is this change to the control flow, that may explain some
perf differences?

[michaeln, 2013-07-10 20:32:20]

https://codereview.chromium.org/18398002/diff/64001/Source/bindings/v8/Except...
File Source/bindings/v8/ExceptionState.cpp (right):

https://codereview.chromium.org/18398002/diff/64001/Source/bindings/v8/Except...
Source/bindings/v8/ExceptionState.cpp:51:
V8ThrowException::throwTypeError(message, m_isolate);
ditto

[arv (Not doing code reviews), 2013-07-10 20:36:07]

On 2013/07/10 20:32:20, michaeln wrote:
>
https://codereview.chromium.org/18398002/diff/64001/Source/bindings/v8/Except...
> File Source/bindings/v8/ExceptionState.cpp (right):
> 
>
https://codereview.chromium.org/18398002/diff/64001/Source/bindings/v8/Except...
> Source/bindings/v8/ExceptionState.cpp:51:
> V8ThrowException::throwTypeError(message, m_isolate);
> ditto

Cancelling commit bot while I investigate to make sure that the user code does
not get called until the C++ code is done.

[arv (Not doing code reviews), 2013-07-10 20:43:20]

On 2013/07/10 20:31:01, michaeln wrote:
>
https://codereview.chromium.org/18398002/diff/64001/Source/bindings/v8/Except...
> File Source/bindings/v8/ExceptionState.cpp (right):
> 
>
https://codereview.chromium.org/18398002/diff/64001/Source/bindings/v8/Except...
> Source/bindings/v8/ExceptionState.cpp:43:
V8ThrowException::setDOMException(ec,
> message, m_isolate);
> Does this call invoke script?
> 
> If so, i think this may change the control flow in ways that might not be
good.
> The old way, script wasn't invoked until the c++ stack unwound up to very near
> when v8 crossed over to c++. The thing is re-entrancy. Once script is running
it
> can then call back into c++. In the new way, we'll be reentering c++ prior to
> having unwound.
> 
> There might be some stack overflow opportunities here?
> 
> Also, assuming there is this change to the control flow, that may explain some
> perf differences?

in v8.h:

 * Schedules an exception to be thrown when returning to JavaScript.

however,

 * When an
 * exception has been scheduled it is illegal to invoke any JavaScript
 * operation; the caller must return immediately and only after the exception
 * has been handled does it become legal to invoke JavaScript operations.
 */

so there is room for the C++ side to do bad things if, for example, the C++ does
something like

v8::ThrowException()
callUserJavaScript()

we do not do this today but it does widen the window for when we can do bad
things. Notice that this error could already happen today because we call
v8::ThrowException() in other places outside the binding code.

[commit-bot: I haz the power, 2013-07-10 20:43:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arv@chromium.org/18398002/64001

[commit-bot: I haz the power, 2013-07-10 23:09:45]

Change committed as 153938