RESOLVED WONTFIX 85174
Chromium tests: "Add support for the Blob constructor" [r115582] regressed blob layout tests
https://bugs.webkit.org/show_bug.cgi?id=85174
Summary Chromium tests: "Add support for the Blob constructor" [r115582] regressed bl...
Pavel Feldman
Reported 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
Attachments
Patch (8.22 KB, patch)
2012-06-04 02:53 PDT, Kinuko Yasuda
no flags
Crash Fix (3.10 KB, patch)
2012-06-04 23:32 PDT, Kinuko Yasuda
haraken: review+
Eric Seidel (no email)
Comment 2 2012-04-30 12:09:56 PDT
Why not CC the authors of that change?
Joshua Bell
Comment 3 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.
Li Yin
Comment 4 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));
Li Yin
Comment 5 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.
Li Yin
Comment 6 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.
Kinuko Yasuda
Comment 7 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...
Kinuko Yasuda
Comment 8 2012-06-04 02:53:38 PDT
Kentaro Hara
Comment 9 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?
Kinuko Yasuda
Comment 10 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?
Kinuko Yasuda
Comment 11 2012-06-04 23:32:59 PDT
Created attachment 145702 [details] Crash Fix Fixing CRASH bug first h. (Thanks to Li Yin)
Kinuko Yasuda
Comment 12 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)
Kentaro Hara
Comment 13 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?
Kinuko Yasuda
Comment 14 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.)
Kentaro Hara
Comment 15 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!
Kinuko Yasuda
Comment 16 2012-06-05 03:57:21 PDT
Kinuko Yasuda
Comment 17 2012-06-05 05:01:55 PDT
(More patches to come)
Kinuko Yasuda
Comment 18 2012-06-05 10:59:44 PDT
Kentaro Hara
Comment 19 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!
Kentaro Hara
Comment 20 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.)
Stephen Chenney
Comment 21 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.
Note You need to log in before you can comment on or make changes to this bug.