Bug 54249 – Throwing in an IndexedDB error or success event should lead to the transaction aborting

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

Throwing in an IndexedDB error or success event should lead to the transaction aborting

[Jeremy Orlow]

Created attachment 82054 [details]
Patch

[Jeremy Orlow]

please review

[Jeremy Orlow]

Created attachment 82057 [details]
Patch

[Nate Chapin]

Comment on attachment 82057 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82057&action=review

> Source/WebCore/dom/EventTarget.h:154
> +        virtual void uncaughtExceptionInEventHandler();

Is there any way to avoid having this function show up all the way down in EventTarget?  Alternatively, is it likely other EventTargets will find this method useful?

[Jeremy Orlow]

Comment on attachment 82057 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82057&action=review

>> Source/WebCore/dom/EventTarget.h:154
>> +        virtual void uncaughtExceptionInEventHandler();
> 
> Is there any way to avoid having this function show up all the way down in EventTarget?  Alternatively, is it likely other EventTargets will find this method useful?

is there a better place for it?  I don't think this is a great one, but I didn't see any other better ones either.

I'm not aware of any other API that would make use of it, but it is possible one would in the future.  Are you concerned about adding yet another vtable entry?  I actually can't think of any way around putting some entry in EventTarget (even if it's something to say that it can be static casted to some other type that would have the method call on it).

[Nate Chapin]

Comment on attachment 82057 [details]
Patch

It's just my little peeve that there are a fair number of these one-off functions in base classes that are empty or hardcoded and only overridden in one subclass.  I can't think of anything better, but I figured I should at least ask :)

[Jeremy Orlow]

Committed r78655: <http://trac.webkit.org/changeset/78655>

[anton muhin]

Comment on attachment 82057 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82057&action=review

> Source/WebCore/bindings/v8/V8AbstractEventListener.cpp:155
> +            innerTryCatch.Reset();

Just FYI: you don't need to Reset innerTryCatch---it catches all the exceptions by default (you need explicit Rethrow to propagate exception caught).

[Mikhail Naganov]

Patch reverted.

[Jeremy Orlow]

Mikhail: in the future, you should post some context in the bug when you're reverting.  I now have absolutely no idea what the failures are and will likely need to re-land just to see what went wrong.

[Jeremy Orlow]

(In reply to comment #10)
> Mikhail: in the future, you should post some context in the bug when you're reverting.  I now have absolutely no idea what the failures are and will likely need to re-land just to see what went wrong.

Never mind....the results were still available.  Please do post a few links when reverting stuff though.  Sorry for the trouble the patch caused.

[Jeremy Orlow]

Mads/Anton: Can you please take a look at this patch and see if you can tell what's wrong with it?  It's causing subtle issues in a lot of layout tests.  Most look something like this though: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Finspector-support%2Funcaught-dom3-exception.html&showExpectations=true&group=%40ToT%20-%20chromium.org

[Mikhail Naganov]

Jeremy, I'm sorry. Next time I will provide links to logs.

[anton muhin]

I'll have a look tomorrow.

[Mads Ager]

Comment on attachment 82057 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82057&action=review

> Source/WebCore/bindings/v8/V8AbstractEventListener.cpp:151
> +            v8::TryCatch innerTryCatch;

I think this is your behavioral change. This inner TryCatch is not verbose which means that the errors are not reported. Does it help to put in innerTryCatch.SetVerbose(true).

[Jeremy Orlow]

(In reply to comment #15)
> (From update of attachment 82057 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82057&action=review
> 
> > Source/WebCore/bindings/v8/V8AbstractEventListener.cpp:151
> > +            v8::TryCatch innerTryCatch;
> 
> I think this is your behavioral change. This inner TryCatch is not verbose which means that the errors are not reported. Does it help to put in innerTryCatch.SetVerbose(true).

Followed up on chat.  Turns out I don't even need the inner one and everything should just work if I check whether it's been caught before the reset.  Clearly I really confused myself on this one.  :-)

[Jeremy Orlow]

Created attachment 82849 [details]
Patch

[Jeremy Orlow]

Committed r78908: <http://trac.webkit.org/changeset/78908>