Bug 98050

Experimentally, to see how things might look.

This turns out to be much more complicated than I expected. Shocking, I know. I'm blocking off some time next week to work through the dependencies.

A horrible first pass almost compiles. :) Cleaning up a few bits and pieces along the way, tracked in bugs blocking this one.

Still working on this? It'd be great to get better error messages!

I am. I thought I'd have a first pass patch up for review by now, but I got sidetracked. It's still very much on my list.

Created attachment 170366 [details] WIP - Adds ExceptionDetail class.

Created attachment 170367 [details] WIP - Bindings & ExceptionBase.

Created attachment 170368 [details] WIP - XMLHTTPRequest + ExceptionDetail.

So. I'm not sure how best to put this together for a conceptual review. Nor am I sure how best to position this for eventual landing... I started with a massive, change the world approach, but that was a mess, and the diff was almost 10 meg. Bleh. The three patches I've uploaded are a minimal start in the direction of replacing ExceptionCode. It's still a bit messy (I'd like to drop the ExceptionCodeDescription hop in the middle), but it should be enough to discuss the idea. * The first patch adds the ExceptionDetail class. I haven't tried to optimize anything at all: it blindly creates new strings regardless of whether or not they're used. I'm hoping this worst-case will give perfbot something to evaluate. :) * The second uses that class to add new methods to the JSC and V8 bindings that accept ExceptionDetail instead of ExceptionCode, and routes ExceptionCode-based calls through the new methods by creating ExceptionDetail objects on the fly. * The third patch would be a fix for bug #97654 in the brave new world these patches are creating. This approach should allow us to slowly convert exceptions to the new hotness without massively unreviewable changes to everything in the world, and, I hope, should make it easy to evaluate the approach I'm suggesting. I'm CCing a few of the folks who chimed in on the webkit-dev thread. Comments are very welcome. :) Thanks!

Created attachment 170371 [details] Patch

(In reply to comment #10) > Created an attachment (id=170371) [details] > Patch Uploaded a monolithic patch for perf tests. Let's see what breaks!

Created attachment 170404 [details] Now with less crashing.

Attachment 170404 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1 LayoutTests/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] LayoutTests/ChangeLog:14: Line contains tab character. [whitespace/tab] [5] Total errors found: 2 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 170404 [details] Now with less crashing. View in context: https://bugs.webkit.org/attachment.cgi?id=170404&action=review > Source/WebCore/dom/ExceptionDetail.h:45 > + void setRedactedDetail(const String& detail) { m_redactedDetail = detail; } What is redacted detail?

(In reply to comment #14) > (From update of attachment 170404 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170404&action=review > > > Source/WebCore/dom/ExceptionDetail.h:45 > > + void setRedactedDetail(const String& detail) { m_redactedDetail = detail; } > > What is redacted detail? Work-in-progress name for the string that can safely be sent to JavaScript. I haven't yet kicked off the discussion on www-dom about that, so I've only stubbed it out here. The names generally need work. Its clearer in ExceptionBase, perhaps that would be better here too.

Created attachment 170568 [details] Perf tests. Hrm. I ran the perf tests twice on the patch just to be sure: I expected this to be slow, but it's quite a bit worse than I thought it would be on some tests. 'Bindings/node-list-access', for instance, regressed about 25%. Other tests have quite a bit of variance between the two runs. 'bindings/gc-tree' was 2% faster on the first, and 20% slower on the second, for example. Weird. Regardless, any optimization is now no longer premature. ;)

Comment on attachment 170404 [details] Now with less crashing. View in context: https://bugs.webkit.org/attachment.cgi?id=170404&action=review When I had considered this I had thought about instead making ExceptionCode the new class and renaming the existing enum to ExceptionCodeType. Then adding operator overloads for operator=(ExceptionCodeType), and another for int conversion. That would save you from having to change hundreds of files just to get it working. > Source/WebCore/bindings/v8/custom/V8XMLHttpRequestCustom.cpp:58 > + if (ed.code()) Should add an UnspecifiedBool operator overload instead. > Source/WebCore/xml/XMLHttpRequest.cpp:1067 > + ed.setCode(INVALID_STATE_ERR); Can we just use an operator overload so we don't need to change every call site?

(In reply to comment #17) > (From update of attachment 170404 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170404&action=review > > When I had considered this I had thought about instead making ExceptionCode the new class and renaming the existing enum to ExceptionCodeType. Then adding operator overloads for operator=(ExceptionCodeType), and another for int conversion. That would save you from having to change hundreds of files just to get it working. That's much cleverer than what I've done. I did think about a similar approach, but ended up leaning towards an explicit setter for clarity. Assignment seemed unclear when dealing with an object with multiple properties. You're right, though, it would make the whole patch simpler. I might need to reevaluate that decision. Thanks!

(In reply to comment #18) > (In reply to comment #17) > > (From update of attachment 170404 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=170404&action=review > > > > When I had considered this I had thought about instead making ExceptionCode the new class and renaming the existing enum to ExceptionCodeType. Then adding operator overloads for operator=(ExceptionCodeType), and another for int conversion. That would save you from having to change hundreds of files just to get it working. > > That's much cleverer than what I've done. I did think about a similar approach, but ended up leaning towards an explicit setter for clarity. Assignment seemed unclear when dealing with an object with multiple properties. > > You're right, though, it would make the whole patch simpler. I might need to reevaluate that decision. Watch out for the Source/WebKit/public/WebExceptionCode.h and chromium dependencies. Most (but not all) of the use cases are from IndexedDB. We're working to eliminate those, though - most of those call sites are already unused with ASSERT(!ec) but the API plumbing hasn't been cleaned up all the way. It would probably be okay to let WebExceptionCode remain int and ExceptionCode become the structure initially (any codes crossing that API boundary lose details - not a change from current behavior) but it may require a little more operator overloading.

We prefer explicit setter over operator overloading in WebCore.

One more tidbit: in DOM4 exceptions all have code 0 and names. Right now in code like Modules/indexeddb we need to define a IDBDatabaseException that has custom codes for use with ExceptionCode, e.g.: ec = IDBDatabaseException::DATA_ERR ... and somewhere else that maps that to the name property of "DataError" Ideally the ExceptionCode API would allow for easy use of the DOM4-style exceptions, and let you do the equivalent of: ec.setDOMException("DataError", "some message...") ... that results in a DOMException with |code| = 0, |name| = "DataError", and (relevant to this bug) a |message| property. ... While I'm wishing, WebIDL no longer requires that methods be marked with "raises(DOMException)". It would be great to remove that requirement from WebKitIDL as well, especially as it makes binding code generation messy with optional parameters e.g. an IDL foo(optional a, optional b) needs to have an implementation with foo(a, b, ExceptionCode&), foo(a, ExceptionCode&) and foo(ExceptionCode&). So if this were to move to a mechanism where the ExceptionCode object is not passed in to methods it would be a win all around. That's probably something best tackled separately, though.

Brief update, since a few folks have asked. I'm still working on this, but its been on the back burner the last 2-3 weeks due to some other stuff that needed to get done. I put together a version that lazily instantiated the strings (as 'RefPtr<StringImpl>'), which has much better performance than the naive patch up here, but still double-digit perf regressions on a few tests. I need to worm harder at minimizing that impact. I hope to have a decent version of the patch for review next week. Let's see how that goes.

Is there a benefit of RefPtr<StringImpl> over String?

(In reply to comment #23) > Is there a benefit of RefPtr<StringImpl> over String? Not significant. I was still seeing 15% drops on a few DOM tests. That was months ago, however... I'm thinking now about reversing the flow: pass a pointer around, and only creating the exception object if an exception happens, rather than incurring the cost of creating an object on every call.

Comment on attachment 170404 [details] Now with less crashing. Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

We’ve dome something similar to this with the new Exception class and ExceptionOr. So we won’t purse this specific approach.