Bug 13517 – DOM Exception 8 in finance.aol.com sub-page

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Charles Gaudette]

Using WebKit r21131

Terminal, and the JavaScript Console, report two back-to-back lines:
<DATE> <TIME> Safari[PID] ERROR : NOT_FOUND_ERR: DOM Exception 8
<DATE> <TIME> Safari[PID] ERROR : NOT_FOUND_ERR: DOM Exception 8

[Mark Rowe (bdash)]

Confirmed.  This doesn't appear to break any functionality of the page, but something is clearly going wrong.

[Charles Gaudette]

Update: Still a bug; with Safari 3.0.2 beta and r24131 build on Mac -- exactly 300 revs later. Clearly one of the problems with is bug is that "ERROR : NOT_FOUND_ERR: DOM Exception 8" is not a very helpful message.

[David Kilzer (:ddkilzer)]

This is not a regression as the same thing happens with Safari 2.0.4 (419.3) with its original WebKit on Mac OS 10.4.10 (8R218).

[Alexey Proskuryakov]

Created attachment 15487 [details]
reduction

[Cameron Zwarich (cpst)]

Created attachment 15495 [details]
Proposed patch

Here is a patch that fixes the bug.

The problem was in VarDeclNode::evaluate. If a binding already exists in the shallowest scope, then a var declaration should do nothing, except possibly assign to it if there is an initializer. The check was there, but only for var declarations without initializers. Hence, when there was already a binding in the shallowest scope from the combination of try / throw, it incorrectly did not assign to it, and instead caused some strange behaviour.

This patch also fixes bug 14237 and the test js1_5/Scope/regress-185485.js, which does not include any examples with 'try' and 'catch', only examples using 'with'. Perhaps we should also include Alexey's example as an additional regression test.

In the patched expected.html there is a recorded failure of the test ecma/Date/15.9.5.12-6.js, but this has been failing for a while. Should have I edited this out of the diff somehow, or simply not included the changed file?

[Cameron Zwarich (cpst)]

Created attachment 15499 [details]
Revised proposed patch

Here is a revised version of my patch. It fixes one coding style issue, and a regression on the test fast/js/const (the build was broken, so I submitted it only using the old JS tests).

I added test cases for these two bugs. Alexey asked me why I copied the arguments check that was introduced in the patch for bug 13456 in addition to the first check, so I added test cases to fast/js/vardecl-preserve-arguments that would fail if that second check weren't there.

When fast/js/const failed, I fixed it by adding code to apply the ReadOnly attribute. In the process of doing so, I found a few existing bugs in the handling of const (that this patch neither introduces nor fixes). In particular, the following are allowed:

var x = 1;
const x = 2;

const y = 1;
const y = 2;

There are some more elaborate ones than this, but I will file them all as a separate bug and include a fix. I simply wanted to mention it here because it is more apparent after looking at the code in this patch.

[Oliver Hunt]

Comment on attachment 15499 [details]
Revised proposed patch

You should use ASSERT rather than assert

I'd also prefer a for loop over the do-while in 
+          do {
+              base = *iter;
+              if (base->getPropertySlot(exec, ident, slot))
+     	          break;
+
+             ++iter;
+          } while (iter != end);

The test cases scare the hell out of me, i'd much rather there were no failures in the results -- i believe we have an expected failures dir into which you can put tests that you still expect to fail...

[Mark Rowe (bdash)]

The do/while loop that Oliver mentions is a common pattern in nodes.cpp for walking the scope chain.  I don't agree with changing it to a for loop.

[Cameron Zwarich (cpst)]

Created attachment 15512 [details]
Revised proposed patch

Here is a revised copy of my proposed patch. I changed the assert to an ASSERT, and I ran the JSCore tests in the right time zone so that no new failures were introduced.

[Darin Adler]

Comment on attachment 15512 [details]
Revised proposed patch

r=me

[Mark Rowe (bdash)]

Landed in r24287.

I had to tweak the ChangeLog a bit as they were missing some useful information.  The LayoutTests one was missing half the files that had been touched, and neither had much information about what exactly was being fixed.  It's usually a good idea to include a simple description of the bug being fixed (the Bugzilla title may suffice in some instances), and a brief comment of how the patch fixes it.  Have a quick skim through JavaScriptCore/ChangeLog and WebCore/ChangeLog to get a feel for this.  Another minor nit was that a tab character snuck into nodes.cpp in your patch -- not a big deal as a pre-commit hook catches them, but it's worth keeping in mind.

Thanks for the patch!