Fire window.onerror for uncaught IndexedDB errors

Fire window.onerror for uncaught IndexedDB errors

Capture the call stack when an IDBRequest is made, and if the request
fails with an error, and the error is not prevented, then fire
window.onerror (and worker's global onerror).

Not part of the Indexed DB "v1" spec, but planned for v2 and already
implemented in Firefox.

BUG=302010

[jsbell, 2014-04-18 19:36:31]

haraken@ - This seems like a hacky approach, but I haven't stumbled on anything
better. Any advice?

(I've had this CL kicking around for a while locally since we had a TODO for it 
and dusted it off after talking w/ Moz folks about it. Turns out they already do
something similar in Firefox.)

[haraken, 2014-04-21 02:36:13]

On 2014/04/18 19:36:31, jsbell wrote:
> haraken@ - This seems like a hacky approach, but I haven't stumbled on
anything
> better. Any advice?
> 
> (I've had this CL kicking around for a while locally since we had a TODO for
it 
> and dusted it off after talking w/ Moz folks about it. Turns out they already
do
> something similar in Firefox.)

This looks hacky, but it looks like this CL is doing what you want to do
straightforwardly.

+mkwst: Do you have any idea about FIXMEs in IDBRequest.cpp?

https://codereview.chromium.org/243523003/diff/20001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/243523003/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:79: m_creationStackTrace =
createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);

Is it safe to limit the number of captured stack trace to
maxCallStackSizeToCapture? If there are a lot of stacks and we capture only a
part of the stacks, then we will end up firing window.error on a wrong frame
(which might be malicious).

https://codereview.chromium.org/243523003/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:487: if
(executionContext()->shouldSanitizeScriptError(frame.sourceURL(), corsStatus)) {

Is it OK to use the executionContext of this IDBRequest object? Or should we use
an executionContext of the |frame|?

[jsbell, 2014-04-21 16:28:37]

https://codereview.chromium.org/243523003/diff/20001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/243523003/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:79: m_creationStackTrace =
createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
On 2014/04/21 02:36:13, haraken wrote:
> 
> Is it safe to limit the number of captured stack trace to
> maxCallStackSizeToCapture? 

Is it possible to capture more? Maybe I'm misunderstanding that enum's purpose.

> If there are a lot of stacks and we capture only a
> part of the stacks, then we will end up firing window.error on a wrong frame
> (which might be malicious).

Is the content of the stack (other than the current frame) used for security
decisions?

https://codereview.chromium.org/243523003/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:487: if
(executionContext()->shouldSanitizeScriptError(frame.sourceURL(), corsStatus)) {
On 2014/04/21 02:36:13, haraken wrote:
> 
> Is it OK to use the executionContext of this IDBRequest object? Or should we
use
> an executionContext of the |frame|?

Great question. Anyone?

https://codereview.chromium.org/243523003/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:490: // FIXME: Firefox just uses error's
name. Should we include the message?
And for the record: I plan to keep the message, especially as our messages are
pretty informative. I don't see a big compat concern here and we can hammer it
out in the IDBv2 spec.

[jsbell, 2014-04-21 17:10:14]

Also, apologies for the LayoutTests blather in this CL. No need to look at it,
and if this goes forward I'll likely change some of it from using expectError()
everywhere to judicious use of e.preventDefault(), depending on the specifics of
the test.

[jsbell, 2014-05-23 19:35:13]

mkwest@ - ping? Can you comment on the FIXMEs in IDBRequest.cpp and/or suggest
alternate approaches?

[Mike West, 2014-05-29 09:58:00]

On 2014/05/23 19:35:13, jsbell wrote:
> mkwest@ - ping? Can you comment on the FIXMEs in IDBRequest.cpp and/or suggest
> alternate approaches?

Oh, man, I'm sorry.

I'll take a close look at this tomorrow morning (today's a holiday). My
apologies!

[jsbell, 2014-06-12 17:50:31]

On 2014/05/29 09:58:00, Mike West wrote:
> I'll take a close look at this tomorrow morning (today's a holiday).

Friendly ping. :)

Again, not urgent, but I'm guessing the holiday revelry wiped this from your
brain.

[Mike West, 2014-06-13 09:27:52]

This LGTM. Thanks for putting up with the fact that I'm a terrible, terrible
person. :(

A few comments inline.

https://codereview.chromium.org/243523003/diff/80001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/243523003/diff/80001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:80: m_creationStackTrace =
createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
I'm a little worried about the performance impact of generating a stacktrace
every time we create a request. Do we have any IDB perf tests? If so, watch them
closely after landing this.

If not, we'll never notice anyway, so no worries. ;)

https://codereview.chromium.org/243523003/diff/80001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:505: // FIXME: SharableCrossOrigin or
NotSharableCrossOrigin?
If you set this to "ShareableCrossOrigin", then `onerror` will always include
the stack and error message, regardless of the script that generated them. This
generally happens only when you add a `crossorigin` attribute to a `<script>`
tag, and it is delivered with proper CORS headers.

If you set it to "NotSharableCrossOrigin", then the details will only be
available if `frame.sourceURL()` (which you pass in below) is accessible from
`executionContext()`. That seems correct to me (it's also what we do in
Workers).

https://codereview.chromium.org/243523003/diff/80001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:512: // FIXME: Firefox just uses error's
name. Should we include the message?
We have better messages than Firefox. We should use them.

[jsbell, 2014-06-13 16:58:41]

Thanks for the review!

I'll need to spend some time going through the tests again, figuring out in each
case if they should bubble up to window.onerror get stopped with
e.preventDefault().

https://codereview.chromium.org/243523003/diff/80001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/243523003/diff/80001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:80: m_creationStackTrace =
createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
On 2014/06/13 09:27:52, Mike West wrote:
> I'm a little worried about the performance impact of generating a stacktrace
> every time we create a request. Do we have any IDB perf tests? If so, watch
them
> closely after landing this.
> 
> If not, we'll never notice anyway, so no worries. ;)

We do have perf tests and I'm worried. 

Since it's not urgent I'll wait for a quiet time to land this, when no other IDB
changes or e.g. oilpan/v8 churn is planned.

https://codereview.chromium.org/243523003/diff/80001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:505: // FIXME: SharableCrossOrigin or
NotSharableCrossOrigin?
On 2014/06/13 09:27:52, Mike West wrote:
> If you set this to "ShareableCrossOrigin", then `onerror` will always include
> the stack and error message, regardless of the script that generated them.
This
> generally happens only when you add a `crossorigin` attribute to a `<script>`
> tag, and it is delivered with proper CORS headers.
> 
> If you set it to "NotSharableCrossOrigin", then the details will only be
> available if `frame.sourceURL()` (which you pass in below) is accessible from
> `executionContext()`. That seems correct to me (it's also what we do in
> Workers).

Thanks for the explanation/confirmation!

https://codereview.chromium.org/243523003/diff/80001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:512: // FIXME: Firefox just uses error's
name. Should we include the message?
On 2014/06/13 09:27:52, Mike West wrote:
> We have better messages than Firefox. We should use them.

I thought you'd say that. :)

[jsbell, 2014-06-18 18:46:45]

ericu@, cmumford@ - can you take a look?

I tried to make a reasonable choice in the tests about whether to let errors
bubble up unprevented or not - generally, preventing them unless the test
explicitly requires them not to be. In most cases, the prevented errors are from
open request failures (since we have lots of tests around upgrade) and
preventing those errors doesn't stop the open from failing.

Our evalAndLog() pattern unfortunately makes many of the logged source line
numbers be '1'. :(

If everything looks good, I plan to try landing this after the M37 cut (i.e. not
this week) since we may have to revert due to perf impact.

[cmumford, 2014-06-18 21:09:29]

On 2014/06/18 18:46:45, jsbell wrote:
> ericu@, cmumford@ - can you take a look?
> 
> I tried to make a reasonable choice in the tests about whether to let errors
> bubble up unprevented or not - generally, preventing them unless the test
> explicitly requires them not to be. In most cases, the prevented errors are
from
> open request failures (since we have lots of tests around upgrade) and
> preventing those errors doesn't stop the open from failing.
> 
> Our evalAndLog() pattern unfortunately makes many of the logged source line
> numbers be '1'. :(
> 
> If everything looks good, I plan to try landing this after the M37 cut (i.e.
not
> this week) since we may have to revert due to perf impact.

By "prevented" do you mean "propagation is stopped"?

[jsbell, 2014-06-18 21:11:45]

On 2014/06/18 21:09:29, cmumford wrote:
> On 2014/06/18 18:46:45, jsbell wrote:
> > ericu@, cmumford@ - can you take a look?
> > 
> > I tried to make a reasonable choice in the tests about whether to let errors
> > bubble up unprevented or not - generally, preventing them unless the test
> > explicitly requires them not to be. In most cases, the prevented errors are
> from
> > open request failures (since we have lots of tests around upgrade) and
> > preventing those errors doesn't stop the open from failing.
> > 
> > Our evalAndLog() pattern unfortunately makes many of the logged source line
> > numbers be '1'. :(
> > 
> > If everything looks good, I plan to try landing this after the M37 cut (i.e.
> not
> > this week) since we may have to revert due to perf impact.
> 
> By "prevented" do you mean "propagation is stopped"?

Well, preventDefault() vs. stopPropagation(). Hrm, which is a good point. I
wonder if I'm keying off the wrong thing...

[jsbell, 2014-06-18 21:20:43]

On 2014/06/18 21:11:45, jsbell wrote:
> On 2014/06/18 21:09:29, cmumford wrote:
> > On 2014/06/18 18:46:45, jsbell wrote:
> > > ericu@, cmumford@ - can you take a look?
> > > 
> > > I tried to make a reasonable choice in the tests about whether to let
errors
> > > bubble up unprevented or not - generally, preventing them unless the test
> > > explicitly requires them not to be. In most cases, the prevented errors
are
> > from
> > > open request failures (since we have lots of tests around upgrade) and
> > > preventing those errors doesn't stop the open from failing.
> > > 
> > > Our evalAndLog() pattern unfortunately makes many of the logged source
line
> > > numbers be '1'. :(
> > > 
> > > If everything looks good, I plan to try landing this after the M37 cut
(i.e.
> > not
> > > this week) since we may have to revert due to perf impact.
> > 
> > By "prevented" do you mean "propagation is stopped"?
> 
> Well, preventDefault() vs. stopPropagation(). Hrm, which is a good point. I
> wonder if I'm keying off the wrong thing...

Firefox suppresses the error going to window.onerror if `preventDefault()` is
called; `stopPropagation()` has no effect:

>> window.onerror = function(a,b,c) { console.info('window.onerror fired: %s
(%s:%d)', a, b, c); };

>> indexedDB.open('db'+Date.now()).onupgradeneeded = function(e) { var db =
e.target.result; var s = db.createObjectStore('s'); s.put(0,0); s.add(0, 0); };

"window.onerror fired: ConstraintError (debugger eval code:1)"

>> indexedDB.open('db'+Date.now()).onupgradeneeded = function(e) { var db =
e.target.result; var s = db.createObjectStore('s'); s.put(0,0); s.add(0,
0).onerror = function(e) {e.preventDefault()}; };

(nothing)

... which this patch matches. Yay!

So: by "prevented" I mean "called event.preventDefault()"

[jsbell, 2014-06-18 21:21:41]

Er, forgot to copy/paste in this case:

>> indexedDB.open('db'+Date.now()).onupgradeneeded = function(e) { var db =
e.target.result; var s = db.createObjectStore('s'); s.put(0,0); s.add(0,
0).onerror = function(e) {e.stopPropagation()}; };

"window.onerror fired: ConstraintError (debugger eval code:1)"

[cmumford, 2014-06-18 21:32:37]

https://codereview.chromium.org/243523003/diff/120001/LayoutTests/storage/ind...
File LayoutTests/storage/indexeddb/resources/persistence.js (right):

https://codereview.chromium.org/243523003/diff/120001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/resources/persistence.js:16: evt.preventDefault();
Do we also need to call preventDefault in any Chromium tests to avoid breaking
them before this CL lands in Blink?

https://codereview.chromium.org/243523003/diff/120001/Source/modules/indexedd...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/243523003/diff/120001/Source/modules/indexedd...
Source/modules/indexeddb/IDBRequest.cpp:80: m_creationStackTrace =
createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
I see that in some places in Blink we don't allow (which is the default) an
empty call stack. What's the rationale to allow it here?

Also, what do you think the performance impact will be to capturing this call
stack every time?

[jsbell, 2014-06-18 22:28:46]

Yes, I should run the perf tests locally...

https://codereview.chromium.org/243523003/diff/120001/LayoutTests/storage/ind...
File LayoutTests/storage/indexeddb/resources/persistence.js (right):

https://codereview.chromium.org/243523003/diff/120001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/resources/persistence.js:16: evt.preventDefault();
On 2014/06/18 21:32:36, cmumford wrote:
> Do we also need to call preventDefault in any Chromium tests to avoid breaking
> them before this CL lands in Blink?

Nope. At least, they all pass locally.

(I think we only scrape the document content, and the tests don't have anything
that catches window.onerror)

https://codereview.chromium.org/243523003/diff/120001/Source/modules/indexedd...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/243523003/diff/120001/Source/modules/indexedd...
Source/modules/indexeddb/IDBRequest.cpp:80: m_creationStackTrace =
createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
On 2014/06/18 21:32:36, cmumford wrote:
> I see that in some places in Blink we don't allow (which is the default) an
> empty call stack. What's the rationale to allow it here?

Can you give me an example? I must be missing something in my search.

> Also, what do you think the performance impact will be to capturing this call
> stack every time?

No idea. :)

I do know that (1) Firefox manages to do it, and (2) it's something we'll want
for debugging Promises, so if it's too slow we need to solve that.

[ericu, 2014-06-19 00:54:06]

https://codereview.chromium.org/243523003/diff/120001/LayoutTests/storage/ind...
File LayoutTests/storage/indexeddb/resources/odd-strings.js (right):

https://codereview.chromium.org/243523003/diff/120001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/resources/odd-strings.js:41: request.onsuccess =
closeDatabase;
Can you talk me through this change?  Why are we now expecting success here?

https://codereview.chromium.org/243523003/diff/120001/Source/modules/indexedd...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/243523003/diff/120001/Source/modules/indexedd...
Source/modules/indexeddb/IDBRequest.cpp:502: if (!m_preventPropagation &&
dontPreventDefault && event->type() == EventTypeNames::error &&
m_creationStackTrace && m_creationStackTrace->size()) {
Under what circumstances would we not have a stack trace, or would have one, but
of zero size?  Is there any value to reporting the exception even without the
stack trace?

[jsbell, 2014-06-19 01:13:25]

Still owe you an answer to the "empty stack" question.

https://codereview.chromium.org/243523003/diff/120001/LayoutTests/storage/ind...
File LayoutTests/storage/indexeddb/resources/odd-strings.js (right):

https://codereview.chromium.org/243523003/diff/120001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/resources/odd-strings.js:41: request.onsuccess =
closeDatabase;
On 2014/06/19 00:54:05, ericu wrote:
> Can you talk me through this change?  Why are we now expecting success here?

Previously, the test was closing the database in the 'success' callback of a
request made during the upgrade (`request.onsuccess = closeDatabase;`, below).
Closing the database during the upgrade sequence causes the open request to
fail; this was previously silent but now generates window.onerror spew.

Since that complex flow was completely irrelevant to the test itself, I
simplified the flow - closing the database is now done when the open succeeds.
The alternative was to leave that flow intact, but e.preventDefault() or
expectError().

[ericu, 2014-06-19 01:18:40]

https://codereview.chromium.org/243523003/diff/120001/LayoutTests/storage/ind...
File LayoutTests/storage/indexeddb/resources/odd-strings.js (right):

https://codereview.chromium.org/243523003/diff/120001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/resources/odd-strings.js:41: request.onsuccess =
closeDatabase;
On 2014/06/19 01:13:24, jsbell wrote:
> On 2014/06/19 00:54:05, ericu wrote:
> > Can you talk me through this change?  Why are we now expecting success here?
> 
> Previously, the test was closing the database in the 'success' callback of a
> request made during the upgrade (`request.onsuccess = closeDatabase;`, below).
> Closing the database during the upgrade sequence causes the open request to
> fail; this was previously silent but now generates window.onerror spew.
> 
> Since that complex flow was completely irrelevant to the test itself, I
> simplified the flow - closing the database is now done when the open succeeds.
> The alternative was to leave that flow intact, but e.preventDefault() or
> expectError().

OK.

[jsbell, 2014-06-19 17:44:38]

https://codereview.chromium.org/243523003/diff/120001/Source/modules/indexedd...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/243523003/diff/120001/Source/modules/indexedd...
Source/modules/indexeddb/IDBRequest.cpp:502: if (!m_preventPropagation &&
dontPreventDefault && event->type() == EventTypeNames::error &&
m_creationStackTrace && m_creationStackTrace->size()) {
On 2014/06/19 00:54:06, ericu wrote:
> Under what circumstances would we not have a stack trace, or would have one,
but
> of zero size?  Is there any value to reporting the exception even without the
> stack trace?

I believe recent test refactors ensure our unit tests always have an isolate
when IDBRequests are created so I can simplify the test and assert
m_creationStackTrace is non-null.

The stack should only be zero size in unit tests, so we don't need to report
errors if we can't get a stack frame.

[cmumford, 2014-06-23 15:58:04]

https://codereview.chromium.org/243523003/diff/120001/Source/modules/indexedd...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/243523003/diff/120001/Source/modules/indexedd...
Source/modules/indexeddb/IDBRequest.cpp:80: m_creationStackTrace =
createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
On 2014/06/18 22:28:46, jsbell wrote:
> On 2014/06/18 21:32:36, cmumford wrote:
> > I see that in some places in Blink we don't allow (which is the default) an
> > empty call stack. What's the rationale to allow it here?
> 
> Can you give me an example? I must be missing something in my search.
> 
> > Also, what do you think the performance impact will be to capturing this
call
> > stack every time?
> 
> No idea. :)
> 
> I do know that (1) Firefox manages to do it, and (2) it's something we'll want
> for debugging Promises, so if it's too slow we need to solve that.
> 

createScriptCallStackForConsole (used in a few places), and
Source/core/frame/csp/ContentSecurityPolicy.cpp:574. But all others allow an
empty stack.

[jsbell, 2014-07-02 19:40:41]

https://codereview.chromium.org/243523003/diff/120001/Source/modules/indexedd...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/243523003/diff/120001/Source/modules/indexedd...
Source/modules/indexeddb/IDBRequest.cpp:80: m_creationStackTrace =
createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
On 2014/06/23 15:58:04, cmumford wrote:
> On 2014/06/18 22:28:46, jsbell wrote:
> > On 2014/06/18 21:32:36, cmumford wrote:
> > > I see that in some places in Blink we don't allow (which is the default)
an
> > > empty call stack. What's the rationale to allow it here?
> > 
> > Can you give me an example? I must be missing something in my search.
> > 
> > > Also, what do you think the performance impact will be to capturing this
> call
> > > stack every time?
> > 
> > No idea. :)
> > 
> > I do know that (1) Firefox manages to do it, and (2) it's something we'll
want
> > for debugging Promises, so if it's too slow we need to solve that.
> > 
> 
> createScriptCallStackForConsole (used in a few places), and
> Source/core/frame/csp/ContentSecurityPolicy.cpp:574. But all others allow an
> empty stack.

Thanks. It appears (ScriptCallStackFactory.cpp:79) that disallowing an empty
call stack synthesizes a dummy entry in the case where a stack can't be
obtained. While that's not terribly helpful (i.e. you won't get the line
number), I guess it's better to get the onerror than not.

From the comments, the scenario appears to be something like
[0].map(store.add.bind(store))

I'll flip it, and assert that the stack is non-empty.

[jsbell, 2014-07-02 19:59:13]

https://codereview.chromium.org/243523003/diff/120001/Source/modules/indexedd...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/243523003/diff/120001/Source/modules/indexedd...
Source/modules/indexeddb/IDBRequest.cpp:80: m_creationStackTrace =
createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
And FYI, I tried testing w/:

[0, 0].forEach(store.add.bind(store, 0, 0));

which should mean a native call (forEach) into a native call (the bound
store.add) but I'm still getting a valid stack in that case.

[ericu, 2014-07-02 20:18:48]

lgtm

[haraken, 2014-07-02 23:25:39]

LGTM

The implementation looks very weird, but we need to align with the spec and
Firefox...

https://codereview.chromium.org/243523003/diff/180001/Source/modules/indexedd...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/243523003/diff/180001/Source/modules/indexedd...
Source/modules/indexeddb/IDBRequest.cpp:504: if (!m_preventPropagation &&
dontPreventDefault && event->type() == EventTypeNames::error) {

Let's add a comment what this is doing.

[jsbell, 2014-07-02 23:26:59]

On 2014/07/02 23:25:39, haraken wrote:
> LGTM
> 
> The implementation looks very weird, but we need to align with the spec and
> Firefox...
> 
>
https://codereview.chromium.org/243523003/diff/180001/Source/modules/indexedd...
> File Source/modules/indexeddb/IDBRequest.cpp (right):
> 
>
https://codereview.chromium.org/243523003/diff/180001/Source/modules/indexedd...
> Source/modules/indexeddb/IDBRequest.cpp:504: if (!m_preventPropagation &&
> dontPreventDefault && event->type() == EventTypeNames::error) {
> 
> Let's add a comment what this is doing.

Will do.

I'll run perf tests locally before landing, too. (So, next week due to US
holidays)

[jsbell, 2014-07-02 23:28:01]

On 2014/07/02 23:26:59, jsbell wrote:
> On 2014/07/02 23:25:39, haraken wrote:
> > LGTM
> > 
> > The implementation looks very weird, but we need to align with the spec and
> > Firefox...
> > 

I should note that it never actually made it into the "v1" spec. It's on the
list for "v2" but not in any rigorous document yet.

[jsbell, 2014-07-07 21:05:18]

Performance numbers are not good. This is on an Ubuntu z620:

https://docs.google.com/a/chromium.org/spreadsheets/d/1m4jwKs0X89u8FMk3_8zYwv...

Slowdown across the board, 15% to 100%, rising well above the level of noise.

Can't justify landing this as-is; we'll need to profile and see what can be
optimized.

[Mike West, 2014-07-08 13:57:55]

On 2014/07/07 at 21:05:18, jsbell wrote:
> Performance numbers are not good. This is on an Ubuntu z620:
> 
>
https://docs.google.com/a/chromium.org/spreadsheets/d/1m4jwKs0X89u8FMk3_8zYwv...
> 
> Slowdown across the board, 15% to 100%, rising well above the level of noise.
> 
> Can't justify landing this as-is; we'll need to profile and see what can be
optimized.

As a first step, try dropping the number of stack frames that you capture.

+pfeldman might also have ideas, as I think devtools went through some of this
pain as well.

[Mike West, 2014-07-08 13:58:15]

(actually +pfeldman)

[cmumford, 2014-07-08 15:11:22]

On 2014/07/07 21:05:18, jsbell wrote:
> Performance numbers are not good. This is on an Ubuntu z620:
> 
>
https://docs.google.com/a/chromium.org/spreadsheets/d/1m4jwKs0X89u8FMk3_8zYwv...
> 
> Slowdown across the board, 15% to 100%, rising well above the level of noise.
> 
> Can't justify landing this as-is; we'll need to profile and see what can be
> optimized.

Is it possible to reference the somehow reference the caller and then lazily
calculate the stack if needed?

[aandrey, 2014-07-08 18:40:16]

https://codereview.chromium.org/243523003/diff/200001/Source/modules/indexedd...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/243523003/diff/200001/Source/modules/indexedd...
Source/modules/indexeddb/IDBRequest.cpp:80: m_creationStackTrace =
createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, false);
Probably we should collect the stacks only when DevTools is open. We don't
collect stacks in other places (like if exception is thrown) in Blink as well as
in V8 unless DevTools is open.

Also this looks very similar to the Promise uncaught async exception issue:
https://code.google.com/p/v8/issues/detail?id=3093

For example, is there the same issue with this async exception, as to whether
it's actually "uncaught" or not at the time it's fired. One could attach a
req.onerror handler later asynchronously, and probably this handler should be
invoked instead of window.onerror.

[jsbell, 2014-07-08 19:22:15]

On 2014/07/08 18:40:16, aandrey wrote:
>
https://codereview.chromium.org/243523003/diff/200001/Source/modules/indexedd...
> File Source/modules/indexeddb/IDBRequest.cpp (right):
> 
>
https://codereview.chromium.org/243523003/diff/200001/Source/modules/indexedd...
> Source/modules/indexeddb/IDBRequest.cpp:80: m_creationStackTrace =
> createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, false);
> Probably we should collect the stacks only when DevTools is open. We don't
> collect stacks in other places (like if exception is thrown) in Blink as well
as
> in V8 unless DevTools is open.

Hrm. We'd need to collect the call site info (i.e. one stack frame) to get
file/line/column, regardless of DevTools - typically, unexpected errors seen by
window.onerror are logged by web apps and sent up to the server for detecting
unexpected failures seen in the field.

Also, IIRC, window.onerror exposes the Error object which holds the stack info;
changing what's included there based on the DevTools status would be bad. I need
to peek more at what we're doing in Blink, and compare with what Firefox does.

> Also this looks very similar to the Promise uncaught async exception issue:
> https://code.google.com/p/v8/issues/detail?id=3093
> 
> For example, is there the same issue with this async exception, as to whether
> it's actually "uncaught" or not at the time it's fired. One could attach a
> req.onerror handler later asynchronously, and probably this handler should be
> invoked instead of window.onerror.

Unlike Promises, IDBRequests fire traditional DOM events. You can't attach a
success/error handler after the event has fired and see it, but you can
interrogate the readyState and access the value synchronously if it's 'done',
and it only matters if any one handler called preventDefault() to control
whether window.onerror should fire.

If the IDB API were being designed now it would use Promises, but it's too late
for that. :(

[aandrey, 2014-07-08 21:03:05]

On 2014/07/08 19:22:15, jsbell wrote:
> On 2014/07/08 18:40:16, aandrey wrote:
> >
>
https://codereview.chromium.org/243523003/diff/200001/Source/modules/indexedd...
> > File Source/modules/indexeddb/IDBRequest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/243523003/diff/200001/Source/modules/indexedd...
> > Source/modules/indexeddb/IDBRequest.cpp:80: m_creationStackTrace =
> > createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, false);
> > Probably we should collect the stacks only when DevTools is open. We don't
> > collect stacks in other places (like if exception is thrown) in Blink as
well
> as
> > in V8 unless DevTools is open.
> 
> Hrm. We'd need to collect the call site info (i.e. one stack frame) to get
> file/line/column, regardless of DevTools - typically, unexpected errors seen
by
> window.onerror are logged by web apps and sent up to the server for detecting
> unexpected failures seen in the field.
> 
> Also, IIRC, window.onerror exposes the Error object which holds the stack
info;
> changing what's included there based on the DevTools status would be bad. I
need
> to peek more at what we're doing in Blink, and compare with what Firefox does.
> 
> > Also this looks very similar to the Promise uncaught async exception issue:
> > https://code.google.com/p/v8/issues/detail?id=3093
> > 
> > For example, is there the same issue with this async exception, as to
whether
> > it's actually "uncaught" or not at the time it's fired. One could attach a
> > req.onerror handler later asynchronously, and probably this handler should
be
> > invoked instead of window.onerror.
> 
> Unlike Promises, IDBRequests fire traditional DOM events. You can't attach a
> success/error handler after the event has fired and see it, but you can
> interrogate the readyState and access the value synchronously if it's 'done',
> and it only matters if any one handler called preventDefault() to control
> whether window.onerror should fire.
> 
> If the IDB API were being designed now it would use Promises, but it's too
late
> for that. :(

Right. At least we could log the uncaught error in console, like we do for
Promises.

[Yang, 2014-07-09 05:48:33]

On 2014/07/08 21:03:05, aandrey wrote:
> On 2014/07/08 19:22:15, jsbell wrote:
> > On 2014/07/08 18:40:16, aandrey wrote:
> > >
> >
>
https://codereview.chromium.org/243523003/diff/200001/Source/modules/indexedd...
> > > File Source/modules/indexeddb/IDBRequest.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/243523003/diff/200001/Source/modules/indexedd...
> > > Source/modules/indexeddb/IDBRequest.cpp:80: m_creationStackTrace =
> > > createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, false);
> > > Probably we should collect the stacks only when DevTools is open. We don't
> > > collect stacks in other places (like if exception is thrown) in Blink as
> well
> > as
> > > in V8 unless DevTools is open.

This is not entirely true. Whenever an Error object is constructed in V8, we
capture a stack trace. Having devtools open
(V8::SetCaptureStackTraceForUncaughtExceptions set to true) causes V8 to capture
a more detailed stack trace though. 

> > 
> > Hrm. We'd need to collect the call site info (i.e. one stack frame) to get
> > file/line/column, regardless of DevTools - typically, unexpected errors seen
> by
> > window.onerror are logged by web apps and sent up to the server for
detecting
> > unexpected failures seen in the field.
> > 
> > Also, IIRC, window.onerror exposes the Error object which holds the stack
> info;
> > changing what's included there based on the DevTools status would be bad. I
> need
> > to peek more at what we're doing in Blink, and compare with what Firefox
does.
> > 
> > > Also this looks very similar to the Promise uncaught async exception
issue:
> > > https://code.google.com/p/v8/issues/detail?id=3093
> > > 
> > > For example, is there the same issue with this async exception, as to
> whether
> > > it's actually "uncaught" or not at the time it's fired. One could attach a
> > > req.onerror handler later asynchronously, and probably this handler should
> be
> > > invoked instead of window.onerror.
> > 
> > Unlike Promises, IDBRequests fire traditional DOM events. You can't attach a
> > success/error handler after the event has fired and see it, but you can
> > interrogate the readyState and access the value synchronously if it's
'done',
> > and it only matters if any one handler called preventDefault() to control
> > whether window.onerror should fire.
> > 
> > If the IDB API were being designed now it would use Promises, but it's too
> late
> > for that. :(
> 
> Right. At least we could log the uncaught error in console, like we do for
> Promises.

[jsbell, 2014-12-18 23:45:26]

FYI, I gave this a whirl again, changing the size of the captured stack
dynamically depending on whether or not devtools is visible - minor perf
improvement, but it's still a 10%-40% slowdown on the chromium idb_perf tests.
(I don't know if that's because V8 is doing the stack capture behind the scenes,
or if it's just the extra overhead for grabbing even just one frame)

I also checked Firefox; the ErrorEvent (fifth arg to onerror) is always null for
IDB errors, so you don't get a stack anyway (you do get the url/line/col,
though). Interestingly, despite the patch building an error, it's null with this
CL too, so I must be missing something about the error reporting. :P

[aandrey, 2014-12-19 00:40:12]

On 2014/12/18 23:45:26, jsbell wrote:
> FYI, I gave this a whirl again, changing the size of the captured stack
> dynamically depending on whether or not devtools is visible - minor perf
> improvement, but it's still a 10%-40% slowdown on the chromium idb_perf tests.
> (I don't know if that's because V8 is doing the stack capture behind the
scenes,
> or if it's just the extra overhead for grabbing even just one frame)
> 
> I also checked Firefox; the ErrorEvent (fifth arg to onerror) is always null
for
> IDB errors, so you don't get a stack anyway (you do get the url/line/col,
> though). Interestingly, despite the patch building an error, it's null with
this
> CL too, so I must be missing something about the error reporting. :P

This is because you do not create a JS error object.
Search for V8ErrorHandler::storeExceptionOnErrorEventWrapper()

If you want the stack, you should create a v8::Exception::Error(), which also
collects the stack trace inside V8 internals.
But this will be very expensive I guess, as V8 does not even bother to optimize
Error code paths.

[aandrey, 2014-12-19 00:40:28]

https://codereview.chromium.org/243523003/diff/260001/Source/modules/indexedd...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/243523003/diff/260001/Source/modules/indexedd...
Source/modules/indexeddb/IDBRequest.cpp:81: m_creationStackTrace =
createScriptCallStack(stackSize, false);
use createScriptCallStackForConsole().
if you need to pass a given ExecutionContext, create a new
createScriptCallStackForConsole() and get rid of explicit
InspectorInstrumentation dependency
I bet the InspectorInstrumentation::hasFrontends() check should be a noticeable
optimization here.

[jsbell, 2014-12-20 00:51:48]

On 2014/12/19 00:40:28, aandrey wrote:
> use createScriptCallStackForConsole().

Thanks. Sadly, no performance improvement. It looks like most of the time is in
the v8::StackTrace::CurrentStackTrace call.

BTW, the perf tests that I'm running show a ~6s delta over 180000 requests, or
33μs/request overhead to capture the trace. Not sure it's gonna get much lower
than that...

[jsbell, 2014-12-20 01:01:02]

For the record, I just tried a quick hack that captures the stack outside the
constructor, i.e.:

IDBRequest* IDBObjectStore::get(...) {
  IDBRequest* request = IDBRequest::create(..);
  m_backend->get(...); // async IPC to the back end
  request->captureStackTrace();
  return request;
}

... thinking that maybe letting the async IPC initiate earlier might mitigate
the overhead. Sadly, it didn't improve the timing.

[blink-reviews, 2014-12-22 19:54:01]

Do you think there might be a performance difference on ARM?

On Fri Dec 19 2014 at 5:01:03 PM <jsbell@chromium.org> wrote:

> For the record, I just tried a quick hack that captures the stack outside
> the
> constructor, i.e.:
>
> IDBRequest* IDBObjectStore::get(...) {
>    IDBRequest* request = IDBRequest::create(..);
>    m_backend->get(...); // async IPC to the back end
>    request->captureStackTrace();
>    return request;
> }
>
> ... thinking that maybe letting the async IPC initiate earlier might
> mitigate
> the overhead. Sadly, it didn't improve the timing.
>
>
>
> https://codereview.chromium.org/243523003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[jsbell, 2015-09-23 17:38:00]

migrated to https://codereview.chromium.org/1362953003/