Bug 148591 - JSCallbackData::invokeCallback() should return the Exception to the caller
Summary: JSCallbackData::invokeCallback() should return the Exception to the caller
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://dom.spec.whatwg.org/#traversal
Keywords:
Depends on:
Blocks: 148415
  Show dependency treegraph
 
Reported: 2015-08-28 14:33 PDT by Chris Dumez
Modified: 2015-08-29 18:12 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.34 KB, patch)
2015-08-28 15:35 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (720.70 KB, application/zip)
2015-08-28 16:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (739.80 KB, application/zip)
2015-08-28 16:54 PDT, Build Bot
no flags Details
Patch (8.33 KB, patch)
2015-08-28 18:43 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-08-28 14:33:26 PDT
JSCallbackData::invokeCallback() calls a callback function. If this function throws an exception, it will report it and clear it on the VM.
However, in the case of NodeFilter, the DOM specification clearly states that we are supposed to rethrow the exception [1].

Once way to support this is to have JSCallbackData::invokeCallback() return the Exception to the caller and let the caller decide what to do with it (i.e. report it or rethrow it).

[1] https://dom.spec.whatwg.org/#traversal

Some more context at https://www.w3.org/Bugs/Public/show_bug.cgi?id=17713#c16
Comment 1 Chris Dumez 2015-08-28 15:35:15 PDT
Created attachment 260185 [details]
Patch
Comment 2 Mark Lam 2015-08-28 16:03:30 PDT
Comment on attachment 260185 [details]
Patch

r=me
Comment 3 Build Bot 2015-08-28 16:33:21 PDT
Comment on attachment 260185 [details]
Patch

Attachment 260185 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/117903

New failing tests:
storage/websql/execute-sql-rowsAffected.html
storage/websql/transaction-success-callback-isolated-world.html
storage/websql/open-database-while-transaction-in-progress.html
storage/websql/transaction-error-callback.html
storage/websql/alter-to-info-table.html
storage/websql/hash-change-with-xhr.html
storage/websql/null-callbacks.html
storage/websql/database-lock-after-reload.html
storage/websql/sql-error-codes.html
storage/websql/executesql-accepts-only-one-statement.html
storage/websql/test-authorizer.html
storage/websql/sql-data-types.html
storage/websql/success-callback.html
storage/websql/change-version-handle-reuse.html
storage/websql/read-transactions-running-concurrently.html
http/tests/storage/callbacks-are-called-in-correct-context.html
storage/websql/statement-success-callback-isolated-world.html
js/dom/exceptions-thrown-in-callbacks.html
storage/websql/multiple-transactions.html
storage/websql/read-and-write-transactions-dont-run-together.html
storage/websql/multiple-databases-garbage-collection.html
storage/websql/statement-error-callback-isolated-world.html
storage/websql/change-version.html
storage/websql/statement-error-callback.html
storage/websql/multiple-transactions-on-different-handles.html
storage/websql/private-browsing-noread-nowrite.html
Comment 4 Build Bot 2015-08-28 16:33:25 PDT
Created attachment 260194 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 5 Build Bot 2015-08-28 16:54:51 PDT
Comment on attachment 260185 [details]
Patch

Attachment 260185 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/117923

New failing tests:
storage/websql/execute-sql-rowsAffected.html
storage/websql/transaction-success-callback-isolated-world.html
storage/websql/statement-error-callback.html
storage/websql/transaction-error-callback.html
storage/websql/alter-to-info-table.html
storage/websql/hash-change-with-xhr.html
storage/websql/null-callbacks.html
storage/websql/database-lock-after-reload.html
storage/websql/sql-error-codes.html
storage/websql/executesql-accepts-only-one-statement.html
storage/websql/test-authorizer.html
storage/websql/sql-data-types.html
storage/websql/success-callback.html
storage/websql/change-version-handle-reuse.html
storage/websql/read-transactions-running-concurrently.html
http/tests/storage/callbacks-are-called-in-correct-context.html
storage/websql/statement-success-callback-isolated-world.html
js/dom/exceptions-thrown-in-callbacks.html
storage/websql/multiple-transactions.html
storage/websql/read-and-write-transactions-dont-run-together.html
storage/websql/statement-error-callback-isolated-world.html
storage/websql/change-version.html
storage/websql/multiple-transactions-on-different-handles.html
Comment 6 Build Bot 2015-08-28 16:54:55 PDT
Created attachment 260196 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 7 Chris Dumez 2015-08-28 18:43:39 PDT
Created attachment 260200 [details]
Patch
Comment 8 WebKit Commit Bot 2015-08-28 19:31:42 PDT
Comment on attachment 260200 [details]
Patch

Clearing flags on attachment: 260200

Committed r189140: <http://trac.webkit.org/changeset/189140>
Comment 9 WebKit Commit Bot 2015-08-28 19:31:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Alexey Proskuryakov 2015-08-28 22:42:47 PDT
This broke bindings tests: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/7914/steps/bindings-generation-tests/logs/stdio

I'm going to land current results as expected (and to once again wonder if having bindings tests is worth the effort).
Comment 11 Chris Dumez 2015-08-29 18:12:00 PDT
(In reply to comment #10)
> This broke bindings tests:
> https://build.webkit.org/builders/
> Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/7914/steps/bindings-
> generation-tests/logs/stdio
> 
> I'm going to land current results as expected (and to once again wonder if
> having bindings tests is worth the effort).

Sorry about that, I completely forgot. And thanks for fixing it for me.