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

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

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.

[Kentaro Hara]

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

Created attachment 112459 [details]
Patch

[Adam Barth]

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]

Created attachment 112631 [details]
Patch

[Kentaro Hara]

(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 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 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 on attachment 112631 [details]
Patch

OK, I got it.

[Kentaro Hara]

Created attachment 113439 [details]
patch for commit

[WebKit Review Bot]

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 on attachment 113439 [details]
patch for commit

Clearing flags on attachment: 113439

Committed r99161: <http://trac.webkit.org/changeset/99161>