WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Crash Fix
(3.10 KB, patch)
2012-06-04 23:32 PDT
,
Kinuko Yasuda
haraken
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Pavel Feldman
Comment 1
2012-04-30 03:54:55 PDT
http://trac.webkit.org/changeset/115582
https://bugs.webkit.org/show_bug.cgi?id=84555
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
Created
attachment 145548
[details]
Patch
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
Committed
r119479
: <
http://trac.webkit.org/changeset/119479
>
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
v8::TryCatch issue:
http://code.google.com/p/v8/issues/detail?id=2166
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.
Top of Page
Format For Printing
XML
Clone This Bug