Bug 67980

Summary:

[Chromium/Mac] fast/events/constructors/progress-event-constructor.html is failing

Product:

WebKit

Reporter:

Fumitoshi Ukai <ukai>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dominicc, haraken, haraken, heycam, japhet, sam, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

71340

Attachments:

Description	Flags
Patch	none
Patch	morrita: review+
patch for commit	none

Fumitoshi Ukai

Reported 2011-09-12 19:30:38 PDT

The following layout test is failing on Mac fast/events/constructors/progress-event-constructor.html Probable cause: http://trac.webkit.org/changeset/94946/ http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40DEPS%20-%20chromium.org&tests=fast%2Fevents%2Fconstructors%2Fprogress-event-constructor.html DIFF: Webkit Mac10.6 (CG)(deps) --- /b/build/slave/Webkit_Mac10_6__CG__deps_/build/layout-test-results/fast/events/constructors/progress-event-constructor-expected.txt +++ /b/build/slave/Webkit_Mac10_6__CG__deps_/build/layout-test-results/fast/events/constructors/progress-event-constructor-actual.txt @@ -20,7 +20,7 @@ PASS new ProgressEvent('eventType', { loaded: 9007199254740991 }).loaded is 9007199254740991 PASS new ProgressEvent('eventType', { loaded: 18446744073709551615 }).loaded is 0 PASS new ProgressEvent('eventType', { loaded: 12345678901234567890 }).loaded is 12345678901234567168 -PASS new ProgressEvent('eventType', { loaded: -1 }).loaded is 18446744073709551615 +FAIL new ProgressEvent('eventType', { loaded: -1 }).loaded should be 18446744073709552000. Was 0. PASS new ProgressEvent('eventType', { loaded: NaN }).loaded is 0 PASS new ProgressEvent('eventType', { loaded: 123.45 }).loaded is 123 PASS new ProgressEvent('eventType', { loaded: undefined }).loaded is 0 @@ -41,7 +41,7 @@ PASS new ProgressEvent('eventType', { total: 9007199254740991 }).total is 9007199254740991 PASS new ProgressEvent('eventType', { total: 18446744073709551615 }).total is 0 PASS new ProgressEvent('eventType', { total: 12345678901234567890 }).total is 12345678901234567168 -PASS new ProgressEvent('eventType', { total: -1 }).total is 18446744073709551615 +FAIL new ProgressEvent('eventType', { total: -1 }).total should be 18446744073709552000. Was 0.

Attachments
Patch (7.99 KB, patch) 2011-10-26 00:20 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
Patch (11.11 KB, patch) 2011-10-26 19:02 PDT, Kentaro Hara	morrita: review+	Details Formatted Diff Diff
patch for commit (11.17 KB, patch) 2011-11-03 00:20 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Kentaro Hara

Comment 1 2011-09-13 23:26:47 PDT

> -PASS new ProgressEvent('eventType', { loaded: -1 }).loaded is 18446744073709551615 > +FAIL new ProgressEvent('eventType', { loaded: -1 }).loaded should be 18446744073709552000. Was 0. I am not 100% sure but this failure seems to happen on clang build. - Linux Chromium (V8 + gcc) => PASS - Mac Chromium (V8 + gcc) => PASS - Mac Safari (JSC + gcc) => PASS - Mac Chromium Canary (V8 + clang) => FAIL - Win Chromium Canary (V8 + VC) => PASS I am investigating more in detail...

Kentaro Hara

Comment 2 2011-10-26 00:20:42 PDT

Created attachment 112459 [details] Patch

Adam Barth

Comment 3 2011-10-26 09:48:22 PDT

Comment on attachment 112459 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112459&action=review > Source/WebCore/bindings/v8/OptionsObject.cpp:238 > + // Calculate d % 2^{64}. > + // -2^{64} < fmodValue < 2^{64}. > + double fmodValue = fmod(trunc(d), std::numeric_limits<unsigned long long>::max() + 1.0); > + if (fmodValue >= 0) { > + // 0 <= fmodValue < 2^{64}. > + // 0 <= value < 2^{64}. This cast causes no loss. > + value = static_cast<unsigned long long>(fmodValue); > + } else { > + // -2^{64} < fmodValue < 0. > + // 0 < fmodValueInUnsignedLongLong < 2^{64}. This cast causes no loss. > + unsigned long long fmodValueInUnsignedLongLong = static_cast<unsigned long long>(-fmodValue); > + // -1 < (std::numeric_limits<unsigned long long>::max() - fmodValueInUnsignedLongLong) < 2^{64} - 1. > + // 0 < value < 2^{64}. > + value = std::numeric_limits<unsigned long long>::max() - fmodValueInUnsignedLongLong + 1; > + } This looks cool, but it should be factored into a more general location so that folks can re-use it. If it's V8-specific logic, it should probably go in V8Utilities. If it's more general than V8, it should go in WTF somewhere.

Kentaro Hara

Comment 4 2011-10-26 19:02:12 PDT

Created attachment 112631 [details] Patch

Kentaro Hara

Comment 5 2011-10-27 17:38:10 PDT

(In reply to comment #3) > (From update of attachment 112459 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=112459&action=review > > > Source/WebCore/bindings/v8/OptionsObject.cpp:238 > > + // Calculate d % 2^{64}. > > + // -2^{64} < fmodValue < 2^{64}. > > + double fmodValue = fmod(trunc(d), std::numeric_limits<unsigned long long>::max() + 1.0); > > + if (fmodValue >= 0) { > > + // 0 <= fmodValue < 2^{64}. > > + // 0 <= value < 2^{64}. This cast causes no loss. > > + value = static_cast<unsigned long long>(fmodValue); > > + } else { > > + // -2^{64} < fmodValue < 0. > > + // 0 < fmodValueInUnsignedLongLong < 2^{64}. This cast causes no loss. > > + unsigned long long fmodValueInUnsignedLongLong = static_cast<unsigned long long>(-fmodValue); > > + // -1 < (std::numeric_limits<unsigned long long>::max() - fmodValueInUnsignedLongLong) < 2^{64} - 1. > > + // 0 < value < 2^{64}. > > + value = std::numeric_limits<unsigned long long>::max() - fmodValueInUnsignedLongLong + 1; > > + } > > This looks cool, but it should be factored into a more general location so that folks can re-use it. If it's V8-specific logic, it should probably go in V8Utilities. If it's more general than V8, it should go in WTF somewhere. Done. Moved it to wtf/MathExtras.h.

Hajime Morrita

Comment 6 2011-11-02 10:32:01 PDT

Comment on attachment 112631 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112631&action=review > Source/JavaScriptCore/wtf/MathExtras.h:297 > +inline void doubleToInteger(double d, unsigned long long& value) Why not return value? That can remove all else clauses.

Kentaro Hara

Comment 7 2011-11-02 10:48:54 PDT

Comment on attachment 112631 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112631&action=review >> Source/JavaScriptCore/wtf/MathExtras.h:297 > > Why not return value? That can remove all else clauses. I am thinking of overloading doubleToInteger() in the future, like doubleToInteger(double d, long long& value). Or should we rename this to doubleToUnsignedLongLong() and just return the value?

Hajime Morrita

Comment 8 2011-11-02 13:08:44 PDT

Comment on attachment 112631 [details] Patch OK, I got it.

Kentaro Hara

Comment 9 2011-11-03 00:20:45 PDT

Created attachment 113439 [details] patch for commit

WebKit Review Bot

Comment 10 2011-11-03 01:06:52 PDT

Comment on attachment 113439 [details] patch for commit Rejecting attachment 113439 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: n/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Updating chromium port dependencies using gclient... ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/10190549

WebKit Review Bot

Comment 11 2011-11-03 03:23:27 PDT

Comment on attachment 113439 [details] patch for commit Clearing flags on attachment: 113439 Committed r99161: <http://trac.webkit.org/changeset/99161>

Note You need to log in before you can comment on or make changes to this bug.