Bug 36458 – DOMCoreException needs NoStaticTables modifier

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Eric U.]

Without it, its fields are inaccessible from all but one thread.  If you've got workers and the main thread both throwing exceptions, you'll have issues--methods of the object are inaccessible, and you can at least crash a debug build by enumerating them.  So far I've managed to see worker exceptions .toString returning [object DOMException] instead of the custom string; I'm not sure if anything worse can happen.

It's a one-line fix; I'll have a patch up shortly.

[Alexey Proskuryakov]

It's so wrong that we need to make anything with "DOM" in its name supported in workers. I guess the evil started with XMLHttpRequest and other non-DOM APIs using exceptions that didn't belong to them.

[Eric U.]

Created attachment 51373 [details]
Patch

[Eric U.]

(In reply to comment #1)
> It's so wrong that we need to make anything with "DOM" in its name supported in
> workers. I guess the evil started with XMLHttpRequest and other non-DOM APIs
> using exceptions that didn't belong to them.

No argument here; I think we're stuck with the terminology now.

[Dmitry Titov]

(In reply to comment #1)
> It's so wrong that we need to make anything with "DOM" in its name supported in
> workers. I guess the evil started with XMLHttpRequest and other non-DOM APIs
> using exceptions that didn't belong to them.

DOMString (as it is known in IDL files) probably was the first claw (or victim) of the evil. Alexey, do you suggest some renaming or a new class for the exception used in Workers?

The patch otherwise looks ok to me.

[Alexey Proskuryakov]

Comment on attachment 51373 [details]
Patch

Renaming may make sense - DOMCoreException is just an internal name for WebKit, and I suspect that it was chosen simply to avoid file name conflicts. On the other hand, it's still DOMException, so hiding that fact under a different name would be confusing.

r=me with the understanding that this will be covered by regression tests soon.

[WebKit Commit Bot]

Comment on attachment 51373 [details]
Patch

Rejecting patch 51373 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12532 test cases.
fast/workers/worker-context-multi-port.html -> failed

Exiting early after 1 failures. 8830 tests run.
153.25s total testing time

8829 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
5 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/1206001

[Dmitry Titov]

The test has this in its expected results:

PASS posting a null port did throw: [object DOMException]

Needs new expected results it seems.

[Eric U.]

Created attachment 51525 [details]
Patch

[Eric U.]

Comment on attachment 51525 [details]
Patch

Fixed the test expectation not to rely on the old broken behavior.  Sorry about that!

[WebKit Commit Bot]

Comment on attachment 51525 [details]
Patch

Clearing flags on attachment: 51525

Committed r56462: <http://trac.webkit.org/changeset/56462>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.