Bug 106411 – IndexedDB: Update expected results for IndexedDB layout tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Michael Pruett]

A few layout tests in storage/indexeddb have different output on JSC than on V8. The V8 versions of these tests should be moved to platform/chromium/storage/indexeddb and the JSC output should be installed in storage/indexeddb.

[Michael Pruett]

Created attachment 181835 [details]
Patch

[Benjamin Poulain]

It is a reasonable change but it is very unfortunate.

Do you know what causes the difference between V8 and JSC?
Could we update the test or the test harness to make sure the results are the same?

[Michael Pruett]

(In reply to comment #2)
> It is a reasonable change but it is very unfortunate.
> 
> Do you know what causes the difference between V8 and JSC?
> Could we update the test or the test harness to make sure the results are the same?

Yes the differences in the expected results in these four test cases are caused by differences in error messages between V8 and JSC.

The error message regarding serializing cyclic structures in JSC is "JSON.stringify cannot serialize cyclic structures." (Source/JavaScriptCore/runtime/JSONObject.cpp line 412) while the comparable error message in V8 is "Converting circular structure to JSON" (v8/src/messages.js line 116).

V8 prepends the string "Uncaught " (v8/src/messages.js line 55) to uncaught exceptions whereas JSC does not.

[Joshua Bell]

> The error message regarding serializing cyclic structures in JSC is "JSON.stringify cannot serialize cyclic structures." (Source/JavaScriptCore/runtime/JSONObject.cpp line 412) while the comparable error message in V8 is "Converting circular structure to JSON" (v8/src/messages.js line 116).

We could alter these tests to catch and verify the exception name and code w/o showing the message.
 
> V8 prepends the string "Uncaught " (v8/src/messages.js line 55) to uncaught exceptions whereas JSC does not.

Unfortunately, we really need to keep these exceptions uncaught as that's what's being tested. (And for the record, catching with using window.onerror doesn't count as being "caught", but also does not prevent the message, so that's not helpful.)

Other thoughts?

[Michael Pruett]

The error messages relating to serializing cyclic structures are rather incidental to the functionality being tested:

shouldThrow("JSON.stringify(cyclic_array)");

These lines could be removed without affecting coverage of IndexedDB in the slightest.

[Joshua Bell]

(In reply to comment #5)
> The error messages relating to serializing cyclic structures are rather incidental to the functionality being tested:
> 
> shouldThrow("JSON.stringify(cyclic_array)");
> 
> These lines could be removed without affecting coverage of IndexedDB in the slightest.

Agreed.

[Michael Pruett]

Created attachment 181971 [details]
Patch

I've removed expected results for storage/indexeddb/key-type-array.html from this change since that test has been updated in bug 106472.

It's not ideal for these three IndexedDB layout tests to have slightly different results on JSC and V8, but eliminating these differences might be more trouble than it's worth.

[Joshua Bell]

(In reply to comment #7)
> It's not ideal for these three IndexedDB layout tests to have slightly different results on JSC and V8, but eliminating these differences might be more trouble than it's worth.

Unless anyone has any better ideas, I'm okay with this approach (i.e. different expectations files). 

(Apologies in advance for when us chromium-port folks commit the wrong thing if no  ews bots catch it.)

[Tony Chang]

FWIW, I think there are quite a few fast/js tests that have different results for Chromium because of V8 error messages being different.

[Benjamin Poulain]

(In reply to comment #9)
> FWIW, I think there are quite a few fast/js tests that have different results for Chromium because of V8 error messages being different.

Yes indeed.

I don't have any problem with landing this.

[WebKit Review Bot]

Comment on attachment 181971 [details]
Patch

Clearing flags on attachment: 181971

Committed r139384: <http://trac.webkit.org/changeset/139384>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.