Bug 85174 – Chromium tests: "Add support for the Blob constructor" [r115582] regressed blob layout tests

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Pavel Feldman]

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

[Pavel Feldman]

http://trac.webkit.org/changeset/115582
https://bugs.webkit.org/show_bug.cgi?id=84555

[Eric Seidel (no email)]

Why not CC the authors of that change?

[Joshua Bell]

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]

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]

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]

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]

> 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]

Created attachment 145548 [details]
Patch

[Kentaro Hara]

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]

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]

Created attachment 145702 [details]
Crash Fix

Fixing CRASH bug first h. (Thanks to Li Yin)

[Kinuko Yasuda]

(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 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]

(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]

(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]

Committed r119479: <http://trac.webkit.org/changeset/119479>

[Kinuko Yasuda]

(More patches to come)

[Kinuko Yasuda]

v8::TryCatch issue: http://code.google.com/p/v8/issues/detail?id=2166

[Kentaro Hara]

(In reply to comment #18)
> v8::TryCatch issue: http://code.google.com/p/v8/issues/detail?id=2166

It is now fixed!

[Kentaro Hara]

(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]

Marked LayoutTest bugs, bugs with Chromium IDs, and some others as WontFix. Test failure bugs still are trackable via TestExpectations or disabled unit tests.