Bug 53705 – Viewport parsing no longer accepts "1.0;" value as valid.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joseph Pecoraro]

The new ViewportArguments implementation looks nice and clean, but it has a few changes
which would cause compatibility problems.

The new implementation landed in r67376:
http://trac.webkit.org/changeset/67376

It follows the "css-viewport" spec:
http://people.opera.com/rune/TR/css-viewport/#parsing-algorithm

I think it incorrectly follows the parsing algorithm for numeric values:

>     If a prefix of property-value can be converted to a number using strtod,
>     the value will be that number. The remainder of the string is ignored.

In the case where the value is "1.0;" String::toFloat will return 1.0, but will indicate
an error. The old code would have used "1.0", but the new code ignores the fact
that a valid prefix was given, reports an error, and returns 0.0.

This affects sites like Wikipedia, which incorrectly use ";" as separators in their
viewport meta tag (they should use commas). Example of the current Wikipedia
meta tag is:

> <meta name="viewport" content="width=device-width; initial-scale=1.0; maximum-scale=1.0; user-scalable=0;">

[Kenneth Rohde Christiansen]

I had a patch for this which was rejected. If you search bugzilla you can probably find it, it is similar to what is in Android.

[Joseph Pecoraro]

Thanks for following up Kenneth.

Correct me if I am wrong, but I believe Android's solution was to allow ';' as a
separator. That is not what this is asking.

This is asking that we allow a value such as "1.0x" to return "1.0" and not "0.0". This
looks to be what the old algorithm did, ignoring any String::toFloat error condition,
and just using the parsed float returned from it.

[Joseph Pecoraro]

My suggestion would be to change:

    bool ok;
    float value = valueString.toFloat(&ok);
    if (!ok) {
        reportViewportWarning(document, UnrecognizedViewportArgumentError, keyString);
        return float(0.0);
    }


To something like:

    bool ok;
    float value = valueString.toFloat(&ok);
    if (!ok) {

        // Completely non-numeric. Warn and return.
        if (!isNumeric(valueString[0]) || (valueString[0] == '-' && !isNumeric(valueString[1])) {
            reportViewportWarning => unrecognized value
            return float(0.0);
        }

        // Warn about a semi-numeric value.
        reportViewportWarning => truncated value
    }

Would you agree with those changes?

[Joseph Pecoraro]

isNumeric would probably be isASCIIDigit from wtf/ASCIICType.h.

[Kenneth Rohde Christiansen]

I think that we might need to handle ; separately as many pages are using that as separators, and Android has a patch making it work as a separator. Even facebook uses this.

With your change, how will the following be handled?

"width=320;height=480" 

I guess it would ignore the height.

Apart from my above comments, I agree with your changes.

[Joseph Pecoraro]

(In reply to comment #5)
> I think that we might need to handle ; separately as many pages are using that
> as separators, and Android has a patch making it work as a separator. Even
> facebook uses this.

I think you are referring to:
<http://webkit.org/b/47607> meta tag parser needs to support ; as separator due to Android having made that popular

In that bug, the verdict was that Android had removed the semicolon as a separator,
despite sites like Facebook using it:
https://bugs.webkit.org/show_bug.cgi?id=47607#c6

A google search does show that Fennec made the following claim:
https://developer.mozilla.org/en/Mobile/Viewport_meta_tag#Background

>>    Mobile Safari introduced the "viewport meta tag" to let web developers control
>>    the viewport's size and scale. Many other mobile browsers now support this tag,
>>    although it is not part of any web standard. Apple's documentation does a good
>>    job explaining how web developers can use this tag, but we had to do some
>>    detective work to figure out exactly how to implement it in Fennec. For example,
>>    Safari's documentation says the content is a "comma-delimited list," but existing
>>    browsers and web pages use any mix of commas, semicolons, and spaces as
>>    separators.

If Fennec does support semicolons, then maybe others should follow suit.

All the other descriptions I've found specify comma-separated:
http://docs.blackberry.com/en/developers/deliverables/6176/HTML_ref_meta_564143_11.jsp
http://developer.apple.com/library/safari/#documentation/AppleApplications/Reference/SafariHTMLRef/Articles/MetaTags.html



> With your change, how will the following be handled?
> 
> "width=320;height=480" 
> 
> I guess it would ignore the height.

Correct.


> Apart from my above comments, I agree with your changes.

Excellent! I'm including Grace to see if she can verify that Android did in fact
remove the semicolon as a separator.

[Joseph Pecoraro]

Anyone know a Fennec developer that could comment on this bug? Or where
to open a bug on them to get some clarification on their approach?

[Kenneth Rohde Christiansen]

@Christian, do you know anyone from Mozilla that we can contact about the above?

I personally do not see any issue in supporting ";" as separator and we are currently supporting it in our internal branches as many pages were broken without this support. Unless there are good reasons against supporting it, this will probably end up in actual products. So any pros/cons/comments are very welcome :-)

[Joseph Pecoraro]

Adding Alexey since he had some questions on the previous bug that was closed.

[Joseph Pecoraro]

Oh, and the Viewport meta content parsing algorithm in the css-viewport
spec mentions separating characters, with ";" not being one of them:
http://people.opera.com/rune/TR/css-viewport/#parsing-algorithm

[Alexey Proskuryakov]

It seems fine to disregard trailing junk in the name of future compatibility. Giving equal meaning to commas and semicolons seems weird, I don't think there's precedent to that.

For a different but somewhat similar example, consider HTTP Accept-Language header, and HTTP in general, where comma is a list separator, and semicolon adds parameters: "Accept-Language: da, en-gb;q=0.8, en;q=0.7"

[Joseph Pecoraro]

Android removed semicolon:
http://android.git.kernel.org/?p=platform/external/webkit.git;a=commit;h=3c81bbf75a3efd5f5ac8c7696a0c23c47ce15aa0

[Kenneth Rohde Christiansen]

(In reply to comment #12)
> Android removed semicolon:
> http://android.git.kernel.org/?p=platform/external/webkit.git;a=commit;h=3c81bbf75a3efd5f5ac8c7696a0c23c47ce15aa0

How are they parsing something like "width=100;height=100" then? as width = 100?

[Joseph Pecoraro]

> How are they parsing something like "width=100;height=100" then? as width = 100?

That would be my guess. I don't have an Android device to test that on.

[Joseph Pecoraro]

Created attachment 82738 [details]
[PATCH] Proposed Fix

This implements the fix, as well as improves the console error/warning messages
around this situation. I don't have a Qt/Efl/Gtk machine to run the test, but I
suspect this may slightly change the results of some tests. I also specifically
added a test for this case. I'd hate to just land this and check the watch the bot
for changes. Anyone care to run this on their port? The mac port skips
fast/viewport tests.

Also, see the following for even better warning/error messages: (requires this patch)
<http://webkit.org/b/53707> Viewport Warning/Error Messages Are Now Inaccurate

[WebKit Review Bot]

Attachment 82738 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/dom/ViewportArguments.h:106:  The parameter name "errorCode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 82738 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=82738&action=review

> LayoutTests/ChangeLog:17
> +        FIXME: OTHER TESTS WOULD BE AFFECTED.
> +        fast/viewport/viewport-65.html and potentially others.
> +        ------

Notice that the Qt port already skips a few of the tests.

> Source/WebCore/dom/ViewportArguments.cpp:182
> +static float numericPrefix(const String& keyString, const String& valueString, Document* document, bool* ok)

The valueString and the ok are related to the actual parsing here, where as the keyString and the document are used for error reporting.

What about:

numericPrefix(const String& valueString, bool* ok, ViewportErrorCode* error)

We could then even add a NoError to the ViewportErrorCode if we wanted.

> Source/WebCore/dom/ViewportArguments.cpp:192
> +        if (!isASCIIDigit(firstChar) || (firstChar == '-' && !isASCIIDigit(valueString[1]))) {

What if there is only the firstChar as - and no second one?

[Joseph Pecoraro]

(In reply to comment #17)
> (From update of attachment 82738 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82738&action=review
> 
> > LayoutTests/ChangeLog:17
> > +        FIXME: OTHER TESTS WOULD BE AFFECTED.
> > +        fast/viewport/viewport-65.html and potentially others.
> > +        ------
> 
> Notice that the Qt port already skips a few of the tests.

Good point, I forgot to check that.


> > Source/WebCore/dom/ViewportArguments.cpp:182
> > +static float numericPrefix(const String& keyString, const String& valueString, Document* document, bool* ok)
> 
> The valueString and the ok are related to the actual parsing here, where as the keyString and the document are used for error reporting.
> 
> What about:
> 
> numericPrefix(const String& valueString, bool* ok, ViewportErrorCode* error)
> 
> We could then even add a NoError to the ViewportErrorCode if we wanted.

I see. I would prefer to perform the error reporting in this function, otherwise each of the call sites would need to handle the error code and report the error.

Also because its possible that multiple errors/tips can be reported. The truncation
tip is really just that, a tip, that is not exclusive of other errors/tips. An example
of multiple tips is the truncation tip, which produces a valid value, and the
"use keyword" tip.

  <meta name="viewport" content="width=320;">
    => TIP: "320;" was truncated to numeric prefix
    => TIP: width set to a constant equal to the physical device width, try the "device-width" keyword.

> > Source/WebCore/dom/ViewportArguments.cpp:192
> > +        if (!isASCIIDigit(firstChar) || (firstChar == '-' && !isASCIIDigit(valueString[1]))) {
> 
> What if there is only the firstChar as - and no second one?

Arg, excellent point. I get lucky here its handled in WTFString which returns 0:

    UChar operator[](unsigned index) const
    {
        if (!m_impl || index >= m_impl->length())
            return 0;
        return m_impl->characters()[index];
    }

And isASCIIDigit('\0') is false, since it checks the '0'..'9' range. But still, this
feels "lucky". Would you prefer I explicitly add some bounds checking? I've
almost convinced myself I should =)

[Kenneth Rohde Christiansen]

I understand that you want to do the warning inside 

> +static float numericPrefix(const String& keyString, const String& valueString, Document* document, bool* ok)

But then maybe we should rename it and move the arguments around.

valueString and the ok are related to the actual parsing, and keyString and document are related to the warning.

Hmm, maybe this is just nitpicking, it is fine how it is. It is very localized code.

[Joseph Pecoraro]

(In reply to comment #19)
> > +static float numericPrefix(const String& keyString, const String& valueString, Document* document, bool* ok)
> 
> But then maybe we should rename it and move the arguments around.
> 
> valueString and the ok are related to the actual parsing, and keyString and document are related to the warning.
> 
> Hmm, maybe this is just nitpicking, it is fine how it is. It is very localized code.

Yah, that also crossed my mind. I ordered the arguments the same as all the
other static methods in the file thinking it was better to be consistent.

[Joseph Pecoraro]

Created attachment 83228 [details]
[PATCH] Explicitly Handle String Bounds Checking

- Addressed style comments.
- Addressed Kenneth Rohde Christiansen's String length comment.

[David Kilzer (:ddkilzer)]

Comment on attachment 83228 [details]
[PATCH] Explicitly Handle String Bounds Checking

View in context: https://bugs.webkit.org/attachment.cgi?id=83228&action=review

r=me, but consider removing the unused default statement, and making toFloat() accept an "ignore trailing garbage" flag (if that's even possible or easily done).

> Source/WebCore/dom/ViewportArguments.cpp:192
> +        if (!length || !isASCIIDigit(valueString[0]) || (length > 1 && valueString[0] == '-' && !isASCIIDigit(valueString[1]))) {

Should we allow a unary "+" operator as well?

What about a decimal number without a leading zero?  ".5"?  Or "+.5"?  Let's not go crazy here, but I want you to consider all possibilities.

It's too bad the toFloat() method doesn't have a flag to say "ignore trailing garbage".  That way, if you sent "pure" garbage to it, you'd still end up with a value of "0.0f" (which is what you want in this case; or NaN which you could convert to 0.0f here), otherwise the method tries its best to convert whatever numeric string is passed to it (ignoring trailing garbage).  Then you don't have to reparse the string locally to guess if it started with something numeric.

Otherwise, I think it would be cleaner if this code was pulled out into a static inline method with a descriptive name.  It's a bit hard to read on one line.

> Source/WebCore/dom/ViewportArguments.cpp:365
> +    default:
> +        ASSERT_NOT_REACHED();
> +        return ErrorMessageLevel;

If the switch statement has case statements for every ViewportErrorCode enum, then you don't need a "default" case because a missing enum will cause a compiler warning (and thus an error with -Werror set).

[Joseph Pecoraro]

(In reply to comment #22)
> (From update of attachment 83228 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83228&action=review
> 
> r=me, but consider removing the unused default statement, and making toFloat() accept an "ignore trailing garbage" flag (if that's even possible or easily done).
> 
> > Source/WebCore/dom/ViewportArguments.cpp:192
> > +        if (!length || !isASCIIDigit(valueString[0]) || (length > 1 && valueString[0] == '-' && !isASCIIDigit(valueString[1]))) {
> 
> Should we allow a unary "+" operator as well?
> 
> What about a decimal number without a leading zero?  ".5"?  Or "+.5"?  Let's not go crazy here, but I want you to consider all possibilities.
> 
> It's too bad the toFloat() method doesn't have a flag to say "ignore trailing garbage".  That way, if you sent "pure" garbage to it, you'd still end up with a value of "0.0f" (which is what you want in this case; or NaN which you could convert to 0.0f here), otherwise the method tries its best to convert whatever numeric string is passed to it (ignoring trailing garbage).  Then you don't have to reparse the string locally to guess if it started with something numeric.

Yes, '-', '+' and '.', and whitespace are all valid starting characters.
Maybe it would be best to extend the lower lower string functions.


> Otherwise, I think it would be cleaner if this code was pulled out into a static inline method with a descriptive name.  It's a bit hard to read on one line.
> 
> > Source/WebCore/dom/ViewportArguments.cpp:365
> > +    default:
> > +        ASSERT_NOT_REACHED();
> > +        return ErrorMessageLevel;
> 
> If the switch statement has case statements for every ViewportErrorCode enum, then you don't need a "default" case because a missing enum will cause a compiler warning (and thus an error with -Werror set).

Oh, nice. Thanks!

[Joseph Pecoraro]

> > > Source/WebCore/dom/ViewportArguments.cpp:365
> > > +    default:
> > > +        ASSERT_NOT_REACHED();
> > > +        return ErrorMessageLevel;
> > 
> > If the switch statement has case statements for every ViewportErrorCode enum, then you don't need a "default" case because a missing enum will cause a compiler warning (and thus an error with -Werror set).
> 
> Oh, nice. Thanks!

Hmm, the compiler still yelled at me. So I left it here. I could move it past the switch, which would save us a line of code (no "default:") line.

[Joseph Pecoraro]

Created attachment 83612 [details]
[PATCH] Add didReadNumber to toFloat / toDecimal to clarify if "ok" means garbage at end

I might as well do this right!

Much cleaner from the Viewport side of things. "ok" is false if there was any
error, and didReadNumber is true is there was ever a numeric value parsed.

Valid values also include scientific notation, so "1e3" is valid for 100.
I tested this with a bunch of numbers and prefixes +1, -1, .5, etc, and
it looked good.

[Joseph Pecoraro]

> Valid values also include scientific notation, so "1e3" is valid for 100.

1e3 is 1000. Typo on my part.

[Kenneth Rohde Christiansen]

(In reply to comment #26)
> > Valid values also include scientific notation, so "1e3" is valid for 100.
> 
> 1e3 is 1000. Typo on my part.

Would be nice to make tests for these

[Kenneth Rohde Christiansen]

Comment on attachment 83612 [details]
[PATCH] Add didReadNumber to toFloat / toDecimal to clarify if "ok" means garbage at end

I wonder if these should be separate patches? I'm OK with this change, but it would be nice with some buy-in from someone else before committing this.

[David Kilzer (:ddkilzer)]

(In reply to comment #24)
> > > > Source/WebCore/dom/ViewportArguments.cpp:365
> > > > +    default:
> > > > +        ASSERT_NOT_REACHED();
> > > > +        return ErrorMessageLevel;
> > > 
> > > If the switch statement has case statements for every ViewportErrorCode enum, then you don't need a "default" case because a missing enum will cause a compiler warning (and thus an error with -Werror set).
> > 
> > Oh, nice. Thanks!
> 
> Hmm, the compiler still yelled at me. So I left it here. I could move it past the switch, which would save us a line of code (no "default:") line.

I would rather see the assert and return statements moved out of the switch statement so that you get a warning (error) if a new enum is added that the switch statement is missing.

[David Kilzer (:ddkilzer)]

(In reply to comment #28)
> (From update of attachment 83612 [details])
> I wonder if these should be separate patches? I'm OK with this change, but it would be nice with some buy-in from someone else before committing this.

Are there any stakeholders from other ports that are missing from the CC list?

[Kenneth Rohde Christiansen]

cc'ing GTK and EFL stakeholders.

[Joseph Pecoraro]

Well, this has been ~5 days. I think I'm going to land this now, see what viewport tests fail
on core bots and update their expected results.

[Joseph Pecoraro]

Landed r80012:
http://trac.webkit.org/changeset/80012

Watching the bots.

[Dimitri Glazkov (Google)]

(In reply to comment #33)
> Landed r80012:
> http://trac.webkit.org/changeset/80012
> 
> Watching the bots.

Did you guys mean to land this without expectations?

[Joseph Pecoraro]

> Did you guys mean to land this without expectations?

Yes, I don't have builds of the other ports to produce the expected results.


Also, this looks like it just crashed on the Gtk 64bit port, but I
can't find any crash log / report to get information out of it.
I can either revert this change, or skip the test on Gtk. I'll most
likely revert the change if I can't get any more information about
why this crashed.

[WebKit Review Bot]

http://trac.webkit.org/changeset/80012 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
fast/viewport/viewport-129.html

[Martin Robinson]

(In reply to comment #35)
> > Did you guys mean to land this without expectations?
> 
> Yes, I don't have builds of the other ports to produce the expected results.
> 
> 
> Also, this looks like it just crashed on the Gtk 64bit port, but I
> can't find any crash log / report to get information out of it.
> I can either revert this change, or skip the test on Gtk. I'll most
> likely revert the change if I can't get any more information about
> why this crashed.


Crash reports for GTK+ debug bots are at: http://webkit-bots.igalia.com. Here is the dump for this crash:

[New Thread 19324]
[New Thread 19391]
[New Thread 19393]

warning: Can't read pathname for load map: Input/output error.
Core was generated by `/home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/Programs/DumpR'.
Program terminated with signal 11, Segmentation fault.
#0  0x00007ff3906810d2 in ?? ()

Thread 3 (Thread 19393):
#0  0x00007ff399a7f4d9 in pthread_cond_timedwait@@GLIBC_2.3.2 () from /lib/libpthread.so.0
#1  0x00007ff39b0a54c2 in g_cond_timed_wait_posix_impl (cond=0x21ad334, entered_mutex=0x189, abs_time=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./gthread/gthread-posix.c:242
#2  0x00007ff39abd0d31 in g_async_queue_pop_intern_unlocked (queue=0x21a7230, try=0, end_time=0x7ff3476a7bf0) at /tmp/buildd/glib2.0-2.27.91/./glib/gasyncqueue.c:423
#3  0x00007ff39ac25021 in g_thread_pool_wait_for_new_task (data=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./glib/gthreadpool.c:274
#4  g_thread_pool_thread_proxy (data=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./glib/gthreadpool.c:308
#5  0x00007ff39ac23124 in g_thread_create_proxy (data=0x55b2cf0) at /tmp/buildd/glib2.0-2.27.91/./glib/gthread.c:1897
#6  0x00007ff399a7a8ba in start_thread () from /lib/libpthread.so.0
#7  0x00007ff3997e202d in clone () from /lib/libc.so.6
#8  0x0000000000000000 in ?? ()

Thread 2 (Thread 19391):
#0  0x00007ff399a7f4d9 in pthread_cond_timedwait@@GLIBC_2.3.2 () from /lib/libpthread.so.0
#1  0x00007ff39b0a54c2 in g_cond_timed_wait_posix_impl (cond=0x21ad334, entered_mutex=0x189, abs_time=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./gthread/gthread-posix.c:242
#2  0x00007ff39abd0d31 in g_async_queue_pop_intern_unlocked (queue=0x21a7230, try=0, end_time=0x7ff346ea6bf0) at /tmp/buildd/glib2.0-2.27.91/./glib/gasyncqueue.c:423
#3  0x00007ff39ac25021 in g_thread_pool_wait_for_new_task (data=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./glib/gthreadpool.c:274
#4  g_thread_pool_thread_proxy (data=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./glib/gthreadpool.c:308
#5  0x00007ff39ac23124 in g_thread_create_proxy (data=0x7ff38c0071b0) at /tmp/buildd/glib2.0-2.27.91/./glib/gthread.c:1897
#6  0x00007ff399a7a8ba in start_thread () from /lib/libpthread.so.0
#7  0x00007ff3997e202d in clone () from /lib/libc.so.6
#8  0x0000000000000000 in ?? ()

Thread 1 (Thread 19324):
#0  0x00007ff3906810d2 in ?? ()
#1  0x00007ff39db40fdb in JSC::JSValue::toNumber (this=0x7fff618a7ee0, exec=0x7ff3906810d0) at ../../Source/JavaScriptCore/runtime/JSCell.h:310
#2  0x00007ff39e862595 in JSValueToNumber (ctx=0x7ff3906810d0, value=0x7fff618a8010, exception=0x7fff618a8088) at ../../Source/JavaScriptCore/API/JSValueRef.cpp:274
#3  0x000000000040dc55 in dumpConfigurationForViewportCallback (context=0x7ff3906810d0, function=0x7ff3a040fc10, thisObject=0x7ff3a040f750, argumentCount=2, arguments=0x7fff618a7fe8, exception=0x7fff618a8088) at ../../Tools/DumpRenderTree/LayoutTestController.cpp:155
#4  0x00007ff39e84ba38 in JSC::JSCallbackFunction::call (exec=0x7ff3906810d0) at ../../Source/JavaScriptCore/API/JSCallbackFunction.cpp:67
#5  0x00007ff39e8f06c9 in JSC::cti_op_call_NotJSFunction (args=0x7fff618a81c0) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:2094
#6  0x00007ff39e8eb5ff in JSC::JITThunks::tryCacheGetByID (callFrame=0x7ff390681080, codeBlock=0x7fff618a81c0, returnAddress=..., baseValue=..., propertyName=..., slot=..., stubInfo=0x21b92c0) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:869
#7  0x00007ff39e8be6f3 in JSC::JITCode::execute (this=0x774be98, registerFile=0x1bd2828, callFrame=0x7ff390681040, globalData=0x21b92c0) at ../../Source/JavaScriptCore/jit/JITCode.h:77
#8  0x00007ff39e8bb7d5 in JSC::Interpreter::executeCall (this=0x1bd2810, callFrame=0x794a708, function=0x7ff3a040fa90, callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at ../../Source/JavaScriptCore/interpreter/Interpreter.cpp:844
#9  0x00007ff39e948634 in JSC::call (exec=0x794a708, functionObject=..., callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at ../../Source/JavaScriptCore/runtime/CallData.cpp:38
#10 0x00007ff39db4a813 in WebCore::JSMainThreadExecState::call (exec=0x794a708, functionObject=..., callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at ../../Source/WebCore/bindings/js/JSMainThreadExecState.h:48
#11 0x00007ff39db7d98d in WebCore::JSEventListener::handleEvent (this=0x75f6470, scriptExecutionContext=0x7577a98, event=0x7703f60) at ../../Source/WebCore/bindings/js/JSEventListener.cpp:123
#12 0x00007ff39dd72941 in WebCore::EventTarget::fireEventListeners (this=0x75b24b0, event=0x7703f60, d=0x75b2580, entry=...) at ../../Source/WebCore/dom/EventTarget.cpp:354
#13 0x00007ff39dd727d0 in WebCore::EventTarget::fireEventListeners (this=0x75b24b0, event=0x7703f60) at ../../Source/WebCore/dom/EventTarget.cpp:323
#14 0x00007ff39e0d3038 in WebCore::DOMWindow::dispatchEvent (this=0x75b24b0, prpEvent=..., prpTarget=...) at ../../Source/WebCore/page/DOMWindow.cpp:1578
#15 0x00007ff39e0d314c in WebCore::DOMWindow::dispatchTimedEvent (this=0x75b24b0, event=..., target=0x7577a30, startTime=0x7886898, endTime=0x78868a0) at ../../Source/WebCore/page/DOMWindow.cpp:1590
#16 0x00007ff39e0d2cf8 in WebCore::DOMWindow::dispatchLoadEvent (this=0x75b24b0) at ../../Source/WebCore/page/DOMWindow.cpp:1550
#17 0x00007ff39dd2ab12 in WebCore::Document::dispatchWindowLoadEvent (this=0x7577a30) at ../../Source/WebCore/dom/Document.cpp:3490
#18 0x00007ff39dd254f1 in WebCore::Document::implicitClose (this=0x7577a30) at ../../Source/WebCore/dom/Document.cpp:2076
#19 0x00007ff39e04d98d in WebCore::FrameLoader::checkCallImplicitClose (this=0x1bb18a0) at ../../Source/WebCore/loader/FrameLoader.cpp:891
#20 0x00007ff39e04d760 in WebCore::FrameLoader::checkCompleted (this=0x1bb18a0) at ../../Source/WebCore/loader/FrameLoader.cpp:839
#21 0x00007ff39e04d4cf in WebCore::FrameLoader::finishedParsing (this=0x1bb18a0) at ../../Source/WebCore/loader/FrameLoader.cpp:773
#22 0x00007ff39dd2dcfe in WebCore::Document::finishedParsing (this=0x7577a30) at ../../Source/WebCore/dom/Document.cpp:4239
#23 0x00007ff39df5a494 in WebCore::HTMLTreeBuilder::finished (this=0x7a28c40) at ../../Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2804
#24 0x00007ff39df30fb4 in WebCore::HTMLDocumentParser::end (this=0x75a8c40) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:354
#25 0x00007ff39df310b1 in WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd (this=0x75a8c40) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:363
#26 0x00007ff39df303e1 in WebCore::HTMLDocumentParser::prepareToStopParsing (this=0x75a8c40) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:151
#27 0x00007ff39df310f6 in WebCore::HTMLDocumentParser::attemptToEnd (this=0x75a8c40) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:375
#28 0x00007ff39df311af in WebCore::HTMLDocumentParser::finish (this=0x75a8c40) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:403
#29 0x00007ff39dd25c02 in WebCore::Document::finishParsing (this=0x7577a30) at ../../Source/WebCore/dom/Document.cpp:2243
#30 0x00007ff39e04740f in WebCore::DocumentWriter::endIfNotLoadingMainResource (this=0x78860c0) at ../../Source/WebCore/loader/DocumentWriter.cpp:222
#31 0x00007ff39e047365 in WebCore::DocumentWriter::end (this=0x78860c0) at ../../Source/WebCore/loader/DocumentWriter.cpp:207
#32 0x00007ff39e03c47d in WebCore::DocumentLoader::finishedLoading (this=0x7885fc0) at ../../Source/WebCore/loader/DocumentLoader.cpp:284
#33 0x00007ff39e053a1f in WebCore::FrameLoader::finishedLoading (this=0x1bb18a0) at ../../Source/WebCore/loader/FrameLoader.cpp:2188
#34 0x00007ff39e08403d in WebCore::MainResourceLoader::didFinishLoading (this=0x79f2d30, finishTime=0) at ../../Source/WebCore/loader/MainResourceLoader.cpp:464
#35 0x00007ff39e090231 in WebCore::ResourceLoader::didFinishLoading (this=0x79f2d30, finishTime=0) at ../../Source/WebCore/loader/ResourceLoader.cpp:436
#36 0x00007ff39d9fa73e in WebCore::readCallback (source=0x4db10c0, asyncResult=0x7ff38c024800, data=0x0) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:779
#37 0x00007ff39b752f65 in async_ready_callback_wrapper (source_object=0x4db10c0, res=0x7ff38c024800, user_data=0x0) at /tmp/buildd/glib2.0-2.27.91/./gio/ginputstream.c:470
#38 0x00007ff39b764628 in complete_in_idle_cb_for_thread (_data=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./gio/gsimpleasyncresult.c:812
#39 0x00007ff39abf9362 in g_main_dispatch (context=0x1b272c0) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:2440
#40 g_main_context_dispatch (context=0x1b272c0) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3013
#41 0x00007ff39abfda28 in g_main_context_iterate (context=0x1b272c0, block=<value optimized out>, dispatch=<value optimized out>, self=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3091
#42 0x00007ff39abfdf35 in g_main_loop_run (loop=0x78c9660) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3299
#43 0x00007ff39cb32657 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
#44 0x000000000041e32d in runTest (testPathOrURL=...) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:679
#45 0x000000000041d9bf in runTestingServerLoop () at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:489
#46 0x000000000041faa4 in main (argc=2, argv=0x7fff618a9b68) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1143

[Joseph Pecoraro]

Wow, did fast/viewport tests just change? Apparently this is because I
failed to call layoutTestController.dumpConfigurationForViewport with
enough arguments. I can fix this pretty quickly.

[Joseph Pecoraro]

Arg, yes, this changed in the 5 days between when it was r+ and landed:
http://trac.webkit.org/changeset/79783

Sorry about this. Should be an easy test content fix.

[Martin Robinson]

(In reply to comment #38)
> Wow, did fast/viewport tests just change? Apparently this is because I
> failed to call layoutTestController.dumpConfigurationForViewport with
> enough arguments. I can fix this pretty quickly.

Probably the GTK+ DumpRenderTree should be a more resilient to these kind of things. :) I'll ping the guy who wrote this originally.

[Joseph Pecoraro]

Thanks! Yes, it looks like Kenneth made the change. Not a big deal,
mostly my fault for not having a port to test on.

I will also file a bug on the old-runwebkit-tests which saw:

  Use of uninitialized value in numeric lt (<) at Tools/Scripts/old-run-webkit-tests line 1778.

[Joseph Pecoraro]

Landed test content fix in r80031:
http://trac.webkit.org/changeset/80031

I'll still need to grab expected results off of the bots when they finish.

[Joseph Pecoraro]

Created attachment 84300 [details]
[PATCH] Add Expected Results

Now that the build-bot has generated results, they look as I would expect.
I've copied this from the GTK bot, which did churn through all tests after
the previous fix.

[Kenneth Rohde Christiansen]

Joseph, I don't know if you saw it, but apparently the new IE9 (only?) supports ; as separators.

http://blogs.msdn.com/b/iemobile/archive/2010/11/22/the-ie-mobile-viewport-on-windows-phone-7.aspx

I still think that we are doing the right thing, though.

[Joseph Pecoraro]

Comment on attachment 84300 [details]
[PATCH] Add Expected Results

I'll manually land.

[Joseph Pecoraro]

Landed expected results in r80062:
http://trac.webkit.org/changeset/80062

Thanks!

[WebKit Review Bot]

http://trac.webkit.org/changeset/80062 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
fast/viewport/viewport-112.html
fast/viewport/viewport-121.html
fast/viewport/viewport-122.html
fast/viewport/viewport-125.html
fast/viewport/viewport-129.html
fast/viewport/viewport-35.html
fast/viewport/viewport-46.html
fast/viewport/viewport-52.html
fast/viewport/viewport-53.html
fast/viewport/viewport-54.html
fast/viewport/viewport-55.html
fast/viewport/viewport-66.html
fast/viewport/viewport-67.html
fast/viewport/viewport-68.html
fast/viewport/viewport-69.html
fast/viewport/viewport-70.html
fast/viewport/viewport-71.html
fast/viewport/viewport-72.html
fast/viewport/viewport-73.html
fast/viewport/viewport-74.html
fast/viewport/viewport-75.html
fast/viewport/viewport-77.html
fast/viewport/viewport-78.html
fast/viewport/viewport-79.html
fast/viewport/viewport-83.html
fast/xsl/xslt-mismatched-tags-in-xslt.xml
fast/xsl/xslt-missing-namespace-in-xslt.xml
http/tests/security/xss-DENIED-xsl-document-redirect.xml
http/tests/security/xss-DENIED-xsl-external-entity-redirect.xml
http/tests/xmlviewer/dumpAsText/wml.xml

[Misha]

Should we also support trailing junk for device-width etc.?
Many pages specify "width=device-width;" (foxnews.com for example)