Bug 141859 – REGRESSION(r179429): Can't type comments in Facebook

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Geoffrey Garen]

* SUMMARY
Typing into a FB comment field shows text, but hitting enter does not post the text, nor is the pre-existing text overwritten.  The delete key also does nothing.

* STEPS TO REPRODUCE
1. Load FB & log in with an account.
2. Try making a comment on an existing post in your news feed.
3. Try to post your comment.
4. Try to delete the text you typed.

* RESULTS
Notice odd text behavior.  

Yale Van tracked this down to <http://trac.webkit.org/changeset/179429>.

[Geoffrey Garen]

I think we should roll out <http://trac.webkit.org/changeset/179429> until we've resolved this.

Yusuke, can you prepare the rollout patch? I tried asking webkitbot, but the rollout needs some by-hand editing to work.

[Yusuke Suzuki]

(In reply to comment #1)
> I think we should roll out <http://trac.webkit.org/changeset/179429> until
> we've resolved this.
> 
> Yusuke, can you prepare the rollout patch? I tried asking webkitbot, but the
> rollout needs some by-hand editing to work.

Oops. Thank you. I'll do this.

[Yusuke Suzuki]

Created attachment 247012 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/runtime/NameConstructor.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/NameInstance.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/CMakeLists.txt:591:  Alphabetical sorting problem. "disassembler/UDis86Disassembler.cpp" should be before "disassembler/udis86/udis86_syn.c".  [list/order] [5]
Total errors found: 4 in 51 files


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

[Yusuke Suzuki]

(In reply to comment #1)
> I think we should roll out <http://trac.webkit.org/changeset/179429> until
> we've resolved this.
> 
> Yusuke, can you prepare the rollout patch? I tried asking webkitbot, but the
> rollout needs some by-hand editing to work.

I've created the patch reverting this change.
However, reverted code has some linting errors.
Is it preferable to land this reverting patch manually?

[Build Bot]

Comment on attachment 247012 [details]
Patch

Attachment 247012 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5346000512221184

New failing tests:
inspector/model/parse-script-syntax-tree.html
inspector/css/matched-style-properties.html
inspector/protocol-promise-result.html
inspector/event-listener.html
inspector/test-harness-trivially-works.html
inspector/css/pseudo-element-matches.html
inspector/css/selector-specificity.html
inspector/event-listener-set.html

[Build Bot]

Created attachment 247022 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Yusuke Suzuki]

Oh, inspector already use Symbols in its implementation.
Just skipping tests until we've fixed?
One concern is that WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js uses Symbol.

[Build Bot]

Comment on attachment 247012 [details]
Patch

Attachment 247012 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6387479615635456

New failing tests:
inspector/model/parse-script-syntax-tree.html
inspector/css/matched-style-properties.html
inspector/protocol-promise-result.html
inspector/event-listener.html
inspector/test-harness-trivially-works.html
inspector/css/pseudo-element-matches.html
inspector/css/selector-specificity.html
inspector/event-listener-set.html

[Build Bot]

Created attachment 247029 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Yusuke Suzuki]

(In reply to comment #1)
> I think we should roll out <http://trac.webkit.org/changeset/179429> until
> we've resolved this.
> 
> Yusuke, can you prepare the rollout patch? I tried asking webkitbot, but the
> rollout needs some by-hand editing to work.

After merging,

* https://bugs.webkit.org/show_bug.cgi?id=141106
* https://bugs.webkit.org/show_bug.cgi?id=141351

Facebook comment input functionality seems recovered.
So it seems that,
When there's window.Symbol, facebook's JS use it. But to work with it correctly, it also requires Object.getOwnPropertySymbols and Iterator.next interface additionally.

[Ryosuke Niwa]

(In reply to comment #11)
> (In reply to comment #1)
> > I think we should roll out <http://trac.webkit.org/changeset/179429> until
> > we've resolved this.
> > 
> > Yusuke, can you prepare the rollout patch? I tried asking webkitbot, but the
> > rollout needs some by-hand editing to work.
> 
> After merging,
> 
> * https://bugs.webkit.org/show_bug.cgi?id=141106
> * https://bugs.webkit.org/show_bug.cgi?id=141351
> 
> Facebook comment input functionality seems recovered.
> So it seems that,
> When there's window.Symbol, facebook's JS use it. But to work with it
> correctly, it also requires Object.getOwnPropertySymbols and Iterator.next
> interface additionally.

Is it possible to hide "window.Symbol" in the main world without reverting the whole patch?  That way, Inspector can continue to use the symbol and we can unbreak Facebook.

[Ryosuke Niwa]

Rather, hide it under some runtime setting so that Inspector can enable it.

[Yusuke Suzuki]

(In reply to comment #13)
> Rather, hide it under some runtime setting so that Inspector can enable it.

What do you think about leveraging JSGlobalObject::m_experimentsEnabled?
It works as a runtime flag to switch JS experimental functionality.

[Yusuke Suzuki]

(In reply to comment #14)
> (In reply to comment #13)
> > Rather, hide it under some runtime setting so that Inspector can enable it.
> 
> What do you think about leveraging JSGlobalObject::m_experimentsEnabled?
> It works as a runtime flag to switch JS experimental functionality.

Or,

1. exposing Symbol with private name.
2. importing Symbol into Inspector WebPage like "InspectorFrontendHost"

is just simple.

[Mark Rowe (bdash)]

*** Bug 141580 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Created attachment 247043 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 11 files


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

[Yusuke Suzuki]

Created attachment 247045 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 247045 [details]
Patch

Still checking that inspector works.

[David Kilzer (:ddkilzer)]

<rdar://problem/19837231>

[Yusuke Suzuki]

Comment on attachment 247045 [details]
Patch

Correctly works in inspector context.

[Ryosuke Niwa]

Comment on attachment 247045 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Enable JavaScriptExperiments in inspector context.

I'm not certain if it's a good idea to enable all experimental features in JSC.
Can we add a new runtime flag to just toggle this?

[Yusuke Suzuki]

Comment on attachment 247045 [details]
Patch

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

Thank you for your comment!

>> Source/WebCore/ChangeLog:8
>> +        Enable JavaScriptExperiments in inspector context.
> 
> I'm not certain if it's a good idea to enable all experimental features in JSC.
> Can we add a new runtime flag to just toggle this?

You're right. More fine-grained runtime flagging is preferable.
So I'll introduce runtime flags system instead of reusing this JavaScriptExperiments flag,
since there's no system for runtime flagging in JSC.

[Yusuke Suzuki]

Comment on attachment 247045 [details]
Patch

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

>>> Source/WebCore/ChangeLog:8
>>> +        Enable JavaScriptExperiments in inspector context.
>> 
>> I'm not certain if it's a good idea to enable all experimental features in JSC.
>> Can we add a new runtime flag to just toggle this?
> 
> You're right. More fine-grained runtime flagging is preferable.
> So I'll introduce runtime flags system instead of reusing this JavaScriptExperiments flag,
> since there's no system for runtime flagging in JSC.

And now, experiments boolean value is not used in any place.
So I think replacing it with runtime flags is nice.

[Yusuke Suzuki]

Created attachment 247113 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:45:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 32 files


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

[Yusuke Suzuki]

Created attachment 247117 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:45:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 37 files


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

[Yusuke Suzuki]

Created attachment 247120 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:45:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 37 files


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

[Yusuke Suzuki]

Created attachment 247127 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:45:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 37 files


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

[Yusuke Suzuki]

Comment on attachment 247127 [details]
Patch

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

Added comments.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:378
> +        putDirectWithoutTransition(vm, vm.propertyNames->Symbol, symbolConstructor, DontEnum);

We can switch like this.

> Source/WebKit2/UIProcess/API/C/WKPreferencesRef.h:-155
> -WK_EXPORT bool WKPreferencesGetJavaScriptExperimentsEnabled(WKPreferencesRef preferencesRef);

One concern is this.
In Cocoa, this `JavaScriptExperimentsEnabled` API is hidden as Private. However, C API, `WKPreferencesGetJavaScriptExperimentsEnabled` is exposed.

If dropping `WKPreferencesGetJavaScriptExperimentsEnabled` is not good, I suggest that, adding JavaScriptExperimentsEnabled flag into RuntimeFlags and specially handling it (Don't override it with RuntimeFlags setter functions like `WKPreferencesSetJavaScriptRuntimeFlags`).
This way can merge JavaScriptExperiments into RuntimeFlags system and still maintain the above API. `WKPreferencesSetJavaScriptExperimentsEnabled`.
What do you think of?

> Source/WebKit2/UIProcess/gtk/WebInspectorProxyGtk.cpp:73
> +        JSC::RuntimeFlags::SymbolEnabled

We can specify RuntimeFlags like this.

[Geoffrey Garen]

Comment on attachment 247127 [details]
Patch

r=me

I think it's OK to remove WKPreferencesGetJavaScriptExperimentsEnabled because I think it doesn't have clients.

[Yusuke Suzuki]

(In reply to comment #35)
> Comment on attachment 247127 [details]
> Patch
> 
> r=me
> 
> I think it's OK to remove WKPreferencesGetJavaScriptExperimentsEnabled
> because I think it doesn't have clients.

OK! So I've cq+ :) Thank you for your review.

[Joseph Pecoraro]

Comment on attachment 247127 [details]
Patch

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

> Source/JavaScriptCore/runtime/RuntimeFlags.h:34
> +#define JSC_RUNTIME_FLAG(macro) \
> +    macro(SymbolEnabled)

Nice! I think we can use this to add a runtime flag to enable/disable Promises. Currently in a JSContext (the Cocoa API) Promises don't really work, since there is no concept of a run loop in JSC, and the promises never "fire". Until they work, I would expect Promises to be unavailable.

[Yusuke Suzuki]

Comment on attachment 247127 [details]
Patch

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

>> Source/JavaScriptCore/runtime/RuntimeFlags.h:34
>> +    macro(SymbolEnabled)
> 
> Nice! I think we can use this to add a runtime flag to enable/disable Promises. Currently in a JSContext (the Cocoa API) Promises don't really work, since there is no concept of a run loop in JSC, and the promises never "fire". Until they work, I would expect Promises to be unavailable.

That sounds very nice! And by leveraging runtime flags system, we can implement ES6 aggressively :)
I think ES6_CLASS_SYNTAX can be moved from a compile time flag to a runtime flag.

[WebKit Commit Bot]

Comment on attachment 247127 [details]
Patch

Rejecting attachment 247127 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
p -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCoreTestSupport.build/Objects-normal/x86_64/MockCDM.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCoreTestSupport.build/Objects-normal/x86_64/JSInternalSettingsGenerated.o /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSInternalSettingsGenerated.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.appspot.com/results/5792982053683200

[Yusuke Suzuki]

(In reply to comment #39)
> Comment on attachment 247127 [details]
> Patch
> 
> Rejecting attachment 247127 [details] from commit-queue.
> 
> Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch',
> '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'build',
> '--no-clean', '--no-update', '--build-style=release', '--port=mac']"
> exit_code: 2 cwd: /Volumes/Data/EWS/WebKit
> 
> Last 500 characters of output:
> p -o
> /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/
> WebCoreTestSupport.build/Objects-normal/x86_64/MockCDM.o
> 
> ** BUILD FAILED **
> 
> 
> The following build commands failed:
> 	CompileC
> /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/
> WebCoreTestSupport.build/Objects-normal/x86_64/JSInternalSettingsGenerated.o
> /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/
> JSInternalSettingsGenerated.cpp normal x86_64 c++
> com.apple.compilers.llvm.clang.1_0.compiler
> (1 failure)
> 
> Full output: http://webkit-queues.appspot.com/results/5792982053683200

Hm, in the patched repository, `git grep -i JavaScriptExperiments` only shows ChangeLogs. So it seems that auto generated DerivedSources/WebCore/JSInternalSettingsGenerated.cpp is not cleaned up and used previously generated.

[Yusuke Suzuki]

Comment on attachment 247127 [details]
Patch

re cq+, previous attempt failed because JSInternalSettingsGenerated.cpp is not cleaned up despite Settings.in is updated.

[WebKit Commit Bot]

Comment on attachment 247127 [details]
Patch

Clearing flags on attachment: 247127

Committed r180547: <http://trac.webkit.org/changeset/180547>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

This patch broke several JavaScript tests on Windows:

CONSOLE MESSAGE: line 5: ReferenceError: Can't find variable: Symbol

js/symbol-abstract-equality-comparison.html
js/symbol-abstract-relational-comparison.html
js/symbol-in-map.html
js/symbol-object.html
js/symbol-prototype-is-ordinary-object.html
js/symbol-strict-equality-comparison.html
js/symbol-tostring.html
js/symbols.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 141957

[Yusuke Suzuki]

Created attachment 247208 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit/win/WebPreferences.cpp:828:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/win/WebPreferences.cpp:835:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/win/WebPreferences.h:155:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/win/WebPreferences.h:158:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:45:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 45 files


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

[Yusuke Suzuki]

Created attachment 247209 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit/win/WebPreferences.cpp:828:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/win/WebPreferences.cpp:835:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/win/WebPreferences.h:155:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/win/WebPreferences.h:158:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:45:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 45 files


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

[Yusuke Suzuki]

Created attachment 247210 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit/win/WebPreferences.cpp:828:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/win/WebPreferences.cpp:835:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/win/WebPreferences.h:155:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/win/WebPreferences.h:158:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:45:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 45 files


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

[Yusuke Suzuki]

Comment on attachment 247210 [details]
Patch

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

Submitted the revised patch with Windows support. I've annotated the difference from the previous patch.

> Source/WebKit/win/Interfaces/IWebPreferences.idl:65
> +} WebKitJavaScriptRuntimeFlags;

Added enum to COM IDL.

> Source/WebKit/win/Interfaces/IWebPreferencesPrivate.idl:117
> +    HRESULT setJavaScriptRuntimeFlags([in] unsigned flags);

Added COM interface for RuntimeFlags in IWebPreferencesPrivate.

> Source/WebKit/win/WebPreferenceKeysPrivate.h:51
> +#define WebKitJavaScriptRuntimeFlagsPreferenceKey "WebKitJavaScriptRuntimeFlags"

Key for storing RuntimeFlags into CFDictionary.

> Source/WebKit/win/WebPreferences.cpp:218
> +    CFDictionaryAddValue(defaults, CFSTR(WebKitJavaScriptRuntimeFlagsPreferenceKey), CFSTR("0"));

Default to CFSTR("0").

> Source/WebKit/win/WebPreferences.cpp:840
> +

javaScriptRuntimeFlags getter / setter.

> Source/WebKit/win/WebPreferences.h:158
> +        /* [in] */ unsigned);

Added implementation header.

> Source/WebKit/win/WebView.cpp:5147
> +    settings.setJavaScriptRuntimeFlags(JSC::RuntimeFlags(javaScriptRuntimeFlags));

Transfer RuntimeFlags from Windows specific preference object to WebCore.

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:819
> +    prefsPrivate->setJavaScriptRuntimeFlags(WebKitJavaScriptRuntimeFlagsAllEnabled);

DumpRenderTree in Windows support.

[Brent Fulgham]

Comment on attachment 247210 [details]
Patch

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

Thank you for fixing this! Looks great. r=me.

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:820
>      // Set JS experiments enabled: YES

You can remove this comment, as that option no longer exists (and has been superseded by your new runtime flags)

[WebKit Commit Bot]

Comment on attachment 247210 [details]
Patch

Clearing flags on attachment: 247210

Committed r180570: <http://trac.webkit.org/changeset/180570>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

(In reply to comment #53)
> Comment on attachment 247210 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247210&action=review
> 
> Thank you for fixing this! Looks great. r=me.
> 
> > Tools/DumpRenderTree/win/DumpRenderTree.cpp:820
> >      // Set JS experiments enabled: YES
> 
> You can remove this comment, as that option no longer exists (and has been
> superseded by your new runtime flags)

Thank you for your review and investigation of failure caused by my patch!
Now Windows Buildbot passed its test :)