Bug 67980 - [Chromium/Mac] fast/events/constructors/progress-event-constructor.html is failing
Summary: [Chromium/Mac] fast/events/constructors/progress-event-constructor.html is fa...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 71340
  Show dependency treegraph
 
Reported: 2011-09-12 19:30 PDT by Fumitoshi Ukai
Modified: 2011-11-06 13:25 PST (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Fumitoshi Ukai 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.
Comment 1 Kentaro Hara 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...
Comment 2 Kentaro Hara 2011-10-26 00:20:42 PDT
Created attachment 112459 [details]
Patch
Comment 3 Adam Barth 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.
Comment 4 Kentaro Hara 2011-10-26 19:02:12 PDT
Created attachment 112631 [details]
Patch
Comment 5 Kentaro Hara 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.
Comment 6 Hajime Morrita 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.
Comment 7 Kentaro Hara 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?
Comment 8 Hajime Morrita 2011-11-02 13:08:44 PDT
Comment on attachment 112631 [details]
Patch

OK, I got it.
Comment 9 Kentaro Hara 2011-11-03 00:20:45 PDT
Created attachment 113439 [details]
patch for commit
Comment 10 WebKit Review Bot 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
Comment 11 WebKit Review Bot 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>