Bug 85174 - Chromium tests: "Add support for the Blob constructor" [r115582] regressed blob layout tests
Summary: Chromium tests: "Add support for the Blob constructor" [r115582] regressed bl...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kinuko Yasuda
URL:
Keywords:
Depends on: 88526
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-30 03:35 PDT by Pavel Feldman
Modified: 2013-04-09 17:06 PDT (History)
16 users (show)

See Also:


Attachments
Patch (8.22 KB, patch)
2012-06-04 02:53 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Crash Fix (3.10 KB, patch)
2012-06-04 23:32 PDT, Kinuko Yasuda
haraken: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2012-04-30 03:35:23 PDT
fast/files/blob-constructor.html = CRASH
fast/dom/HTMLAnchorElement/anchor-download-unset.html = TIMEOUT
fast/filesystem/file-writer-gc-blob.html = TIMEOUT
fast/filesystem/workers/file-writer-gc-blob.html = TEXT
fast/filesystem/workers/file-writer-sync-truncate-extend.html
http/tests/security/filesystem-iframe-from-remote.html = TEXT
storage/indexeddb/noblobs.html = TEXT
Comment 2 Eric Seidel (no email) 2012-04-30 12:09:56 PDT
Why not CC the authors of that change?
Comment 3 Joshua Bell 2012-05-14 12:42:28 PDT
FYI, alecflett@ updated the storage/indexeddb/noblobs.html test in r116337 to use the new Blob() constructor, but didn't remove the expectations line. I'll do so now.
Comment 4 Li Yin 2012-06-03 19:38:08 PDT
The root cause of fast/files/blob-constructor.html = CRASH is
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp#L129
EXCEPTION_BLOCK(String, stringValue, toWebCoreString(item->ToString()));
should be
EXCEPTION_BLOCK(String, stringValue, toWebCoreString(item));
Comment 5 Li Yin 2012-06-03 19:42:21 PDT
In the fast/files/script-tests/blob-constructor.js

var throwingObj = { toString: function() { throw "Error"; } };
shouldThrow("new Blob([throwingObj])", "'Error'");

results in chromium DumpRenderTree crash.
Comment 6 Li Yin 2012-06-03 19:52:36 PDT
Fixing the blob-constructor.html crash is only the first step to remove it from chromium test_expectations.txt.
There are some gaps with JS binding.
1. shouldThrow("new Blob([], {endings:throwingObj})", "'Error'");
2. shouldThrow("new Blob([], {endings:throwingObj1, type:throwingObj2})", "'Error 1'");
3. shouldThrow("new Blob([], {type:throwingObj2, endings:throwingObj1})", "'Error 1'");
4. shouldBe("window.Blob.length", "2");

It seems that Dictionary willn't throw exception.
Both of tryCatchEndings.HasCaught() and tryCatchType.HasCaught() return 0.
Comment 7 Kinuko Yasuda 2012-06-03 21:47:19 PDT
> Fixing the blob-constructor.html crash is only the first step to remove it from chromium test_expectations.txt.
> There are some gaps with JS binding.
> 1. shouldThrow("new Blob([], {endings:throwingObj})", "'Error'");
> 2. shouldThrow("new Blob([], {endings:throwingObj1, type:throwingObj2})", "'Error 1'");
> 3. shouldThrow("new Blob([], {type:throwingObj2, endings:throwingObj1})", "'Error 1'");
> 4. shouldBe("window.Blob.length", "2");
> 
> It seems that Dictionary willn't throw exception.
> Both of tryCatchEndings.HasCaught() and tryCatchType.HasCaught() return 0.

Thanks for digging this, I'd like to take a look at the binding part...
Comment 8 Kinuko Yasuda 2012-06-04 02:53:38 PDT
Created attachment 145548 [details]
Patch
Comment 9 Kentaro Hara 2012-06-04 03:48:10 PDT
Comment on attachment 145548 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145548&action=review

> Source/WebCore/bindings/v8/custom/V8MessageEventCustom.cpp:119
> -    String originArg = v8ValueToWebCoreString(args[4]);
> -    String lastEventIdArg = v8ValueToWebCoreString(args[5]);
> +    EXCEPTION_BLOCK(String, originArg, v8ValueToWebCoreString(args[4]));
> +    EXCEPTION_BLOCK(String, lastEventIdArg, v8ValueToWebCoreString(args[5]));

It seems that this is not the only place where we need to fix. For example, toWebCoreStringWithNullCheck() is used in V8DOMformDataCustom.cpp without being surrounded by EXCEPTION_BLOCK.

Anyone: BTW, isn't there any way to nest TryCatches? As far as I see v8.h and api.cc, it seems that we can register only one TryCatch handler per Isolate, which means that TryCatches are not nest-able. But in that case what is TryCatch::ReThrow() for?
Comment 10 Kinuko Yasuda 2012-06-04 07:10:10 PDT
Ok it looks at least EWS test passes with this approach.

(In reply to comment #9)
> > Source/WebCore/bindings/v8/custom/V8MessageEventCustom.cpp:119
> > -    String originArg = v8ValueToWebCoreString(args[4]);
> > -    String lastEventIdArg = v8ValueToWebCoreString(args[5]);
> > +    EXCEPTION_BLOCK(String, originArg, v8ValueToWebCoreString(args[4]));
> > +    EXCEPTION_BLOCK(String, lastEventIdArg, v8ValueToWebCoreString(args[5]));
> 
> It seems that this is not the only place where we need to fix. For example, toWebCoreStringWithNullCheck() is used in V8DOMformDataCustom.cpp without being surrounded by EXCEPTION_BLOCK.

Thanks, I'll wait a bit more to see if anyone has better idea about nested TryCatch handling before converting all those callsites.

> Anyone: BTW, isn't there any way to nest TryCatches? As far as I see v8.h and api.cc, it seems that we can register only one TryCatch handler per Isolate, which means that TryCatches are not nest-able. But in that case what is TryCatch::ReThrow() for?

As he wrote v8::TryCatch doesn't look nestable to me, but I want to make sure if this assumption is correct and if it's intended behavior.  Does anyone know?
Comment 11 Kinuko Yasuda 2012-06-04 23:32:59 PDT
Created attachment 145702 [details]
Crash Fix

Fixing CRASH bug first h. (Thanks to Li Yin)
Comment 12 Kinuko Yasuda 2012-06-05 00:16:53 PDT
(In reply to comment #11)
> Created an attachment (id=145702) [details]
> Crash Fix
> 
> Fixing CRASH bug first. (Thanks to Li Yin)

The new patch only fixes the CRASH part to start with.  Can someone take a look at it?  (I'll continue fixing the Text mismatches, but the crash one looks worst)
Comment 13 Kentaro Hara 2012-06-05 01:14:13 PDT
Comment on attachment 145702 [details]
Crash Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=145702&action=review

OK, let's land this patch first, and then investigate the nested TryCatch issues.

> Source/WebCore/ChangeLog:8
> +        Fixes CRASH error for string arguments. (Still has TEXT mismatch problems)

Would you elaborate the explanation?
Comment 14 Kinuko Yasuda 2012-06-05 01:53:44 PDT
(In reply to comment #13)
> (From update of attachment 145702 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145702&action=review
> 
> OK, let's land this patch first, and then investigate the nested TryCatch issues.
> 
> > Source/WebCore/ChangeLog:8
> > +        Fixes CRASH error for string arguments. (Still has TEXT mismatch problems)
> 
> Would you elaborate the explanation?

Thanks.  Will be adding description like below:

+        This fixes a crash problem which could happen when the constructor is
+        given an array which contains String-type item(s).
+        There're still some Text mismatches between v8 results and
+        JSC results, most of them are v8 not throwing exception
+        when it is given an object whose toString() method throws
+        exception.  (The issue will be addresse in a separate patch.)
Comment 15 Kentaro Hara 2012-06-05 01:54:45 PDT
(In reply to comment #14)
> Thanks.  Will be adding description like below:
> 
> +        This fixes a crash problem which could happen when the constructor is
> +        given an array which contains String-type item(s).
> +        There're still some Text mismatches between v8 results and
> +        JSC results, most of them are v8 not throwing exception
> +        when it is given an object whose toString() method throws
> +        exception.  (The issue will be addresse in a separate patch.)

Sounds good!
Comment 16 Kinuko Yasuda 2012-06-05 03:57:21 PDT
Committed r119479: <http://trac.webkit.org/changeset/119479>
Comment 17 Kinuko Yasuda 2012-06-05 05:01:55 PDT
(More patches to come)
Comment 18 Kinuko Yasuda 2012-06-05 10:59:44 PDT
v8::TryCatch issue: http://code.google.com/p/v8/issues/detail?id=2166
Comment 19 Kentaro Hara 2012-06-15 08:45:04 PDT
(In reply to comment #18)
> v8::TryCatch issue: http://code.google.com/p/v8/issues/detail?id=2166

It is now fixed!
Comment 20 Kentaro Hara 2012-07-19 10:33:47 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > v8::TryCatch issue: http://code.google.com/p/v8/issues/detail?id=2166
> 
> It is now fixed!

Unfortunately it is now reverted. I discussed the TryCatch bug with the V8 team, and it will take more time to fix it. (The original fix worked fine if all the nested TryCatch objects exist on V8 stack, but didn't work if any TryCatch object exists on heap.)
Comment 21 Stephen Chenney 2013-04-09 17:06:07 PDT
Marked LayoutTest bugs, bugs with Chromium IDs, and some others as WontFix. Test failure bugs still are trackable via TestExpectations or disabled unit tests.