RESOLVED FIXED 196315
Structure::create should call didBecomePrototype()
https://bugs.webkit.org/show_bug.cgi?id=196315
Summary Structure::create should call didBecomePrototype()
Robin Morisset
Reported 2019-03-27 13:48:58 PDT
Otherwise we won't remember to run haveABadTime() when someone adds to them an indexed accessor. I've found a bunch of prototypes for which we forgot doing this. On the advice of Saam, I've also added an extra check that runs in debug mode at the end of JSGlobalObject::finishCreation() to detect any JSObject with a prototype that does not have mayBePrototype(). I verified that this check catches FunctionPrototype without the fix, so it should make sure we don't forget calling didBecomePrototype() in any prototype we add in the future.
Attachments
Patch (19.04 KB, patch)
2019-03-27 13:53 PDT, Robin Morisset
ysuzuki: review+
ews-watchlist: commit-queue-
Archive of layout-test-results from ews116 for mac-highsierra (1.48 MB, application/zip)
2019-03-27 14:56 PDT, EWS Watchlist
no flags
Patch (26.07 KB, patch)
2019-03-27 18:08 PDT, Robin Morisset
saam: review-
Patch (26.01 KB, patch)
2019-04-05 16:03 PDT, Robin Morisset
saam: review+
Archive of layout-test-results from ews114 for mac-highsierra (472.24 KB, application/zip)
2019-04-05 17:28 PDT, EWS Watchlist
no flags
Patch (28.63 KB, patch)
2019-04-09 11:13 PDT, Robin Morisset
no flags
Patch (29.04 KB, patch)
2019-04-09 11:18 PDT, Robin Morisset
ews-watchlist: commit-queue-
Archive of layout-test-results from ews116 for mac-highsierra (1.94 MB, application/zip)
2019-04-09 12:27 PDT, EWS Watchlist
no flags
Patch (29.81 KB, patch)
2019-04-09 16:40 PDT, Robin Morisset
ews-watchlist: commit-queue-
Patch (67.38 KB, patch)
2019-04-09 17:43 PDT, Robin Morisset
ews-watchlist: commit-queue-
Archive of layout-test-results from ews114 for mac-highsierra (1.60 MB, application/zip)
2019-04-09 19:19 PDT, EWS Watchlist
no flags
Patch (68.09 KB, patch)
2019-04-10 10:59 PDT, Robin Morisset
no flags
Patch (63.92 KB, patch)
2019-04-12 17:54 PDT, Robin Morisset
ews-watchlist: commit-queue-
Archive of layout-test-results from ews115 for mac-highsierra (1.64 MB, application/zip)
2019-04-12 18:59 PDT, EWS Watchlist
no flags
Patch (68.59 KB, patch)
2019-04-15 10:42 PDT, Robin Morisset
no flags
Patch (68.48 KB, patch)
2019-04-26 14:41 PDT, Robin Morisset
no flags
Patch (5.51 KB, patch)
2019-05-03 14:01 PDT, Robin Morisset
no flags
Archive of layout-test-results from ews212 for win-future (13.76 MB, application/zip)
2019-05-03 17:11 PDT, EWS Watchlist
no flags
Patch for landing (79.47 KB, patch)
2019-05-08 13:28 PDT, Robin Morisset
no flags
Patch (111.61 KB, patch)
2019-05-10 14:26 PDT, Robin Morisset
no flags
Archive of layout-test-results from ews115 for mac-highsierra (3.95 MB, application/zip)
2019-05-10 16:48 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews214 for win-future (13.47 MB, application/zip)
2019-05-10 23:26 PDT, EWS Watchlist
no flags
Patch (112.47 KB, patch)
2019-06-22 02:16 PDT, Yusuke Suzuki
no flags
Patch (97.24 KB, patch)
2019-06-22 02:27 PDT, Yusuke Suzuki
no flags
Patch (5.98 KB, patch)
2019-06-25 11:57 PDT, Keith Miller
no flags
Robin Morisset
Comment 1 2019-03-27 13:49:34 PDT
Robin Morisset
Comment 2 2019-03-27 13:53:38 PDT
Yusuke Suzuki
Comment 3 2019-03-27 13:58:09 PDT
Comment on attachment 366101 [details] Patch Nice, r=me
EWS Watchlist
Comment 4 2019-03-27 14:56:43 PDT
Comment on attachment 366101 [details] Patch Attachment 366101 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11687701 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 5 2019-03-27 14:56:59 PDT
Created attachment 366108 [details] Archive of layout-test-results from ews116 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-highsierra Platform: Mac OS X 10.13.6
Robin Morisset
Comment 6 2019-03-27 18:08:16 PDT
Created attachment 366136 [details] Patch Added a missing exception check that was causing failures. Also added fixed the wasm prototypes (not sure why they were not found by the prototypeVerifier, maybe they are not always allocated when there is no wasm code?)
Saam Barati
Comment 7 2019-03-27 18:31:17 PDT
Comment on attachment 366136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366136&action=review > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:2104 > + JSValue prototype = object->get(m_exec, m_vm.propertyNames->prototype); > + if (UNLIKELY(scope.exception())) > + scope.clearException(); This isn't what you want. You want JSObject::getPrototypeDirect. This is like asking for o.__proto__. Doing what you're doing is o.prototype
Saam Barati
Comment 8 2019-03-27 18:32:50 PDT
Comment on attachment 366136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366136&action=review > Source/JavaScriptCore/runtime/ObjectPrototype.cpp:72 > + didBecomePrototype(); this is called above.
Saam Barati
Comment 9 2019-03-27 18:33:25 PDT
Comment on attachment 366136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366136&action=review > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:2135 > +#ifndef NDEBUG I prefer: #if !ASSERT_DISABLED
Saam Barati
Comment 10 2019-03-27 18:35:23 PDT
Comment on attachment 366136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366136&action=review > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:2099 > + if (!isJSCellKind(kind) || !static_cast<JSCell*>(cell)->isObject()) > + return IterationStatus::Continue; I think we should also have a check here for Structure*. Something like: if (Structure* structure = jsDynamicCast<Structure*>(cell)) { if (structure->isMonoProto()) { if (JSObject* proto = structure->storedPrototypeObject()) RELEASE_ASSERT(proto->mayBePrototype()); } }
Saam Barati
Comment 11 2019-03-27 18:37:08 PDT
Comment on attachment 366136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366136&action=review >> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:2135 >> +#ifndef NDEBUG > > I prefer: > #if !ASSERT_DISABLED I'd also just move this to init(). I guess there is also a world where we could call this before we GC. That way, we ensure we're always in a consistent state to some degree as we execute programs. Perhaps that's a bit overkill though.
Robin Morisset
Comment 12 2019-04-05 16:03:15 PDT
Created attachment 366853 [details] Patch Fixed the assert. It found several more issues, including FunctionConstructor and several classes related to TypedArrays, that I also fixed. Thanks to Saam for his help with this.
EWS Watchlist
Comment 13 2019-04-05 17:28:14 PDT
Comment on attachment 366853 [details] Patch Attachment 366853 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11786046 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 14 2019-04-05 17:28:15 PDT
Created attachment 366861 [details] Archive of layout-test-results from ews114 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Saam Barati
Comment 15 2019-04-05 17:51:32 PDT
Comment on attachment 366853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366853&action=review r=me > Source/JavaScriptCore/runtime/InternalFunction.h:56 > + { revert > Source/JavaScriptCore/runtime/JSGlobalObject.h:1037 > + revert > Source/JavaScriptCore/runtime/JSObject.cpp:3912 > + revert > Source/JavaScriptCore/runtime/Structure.cpp:214 > + if (prototype.isCell()) > + RELEASE_ASSERT(prototype.getObject()->mayBePrototype()); There is more than one kind of structure constructor. Perhaps do it somewhere where all are covered? Probably in finishCreation. Probably also worth doing in Structure::changePrototypeTransition
Robin Morisset
Comment 16 2019-04-08 14:55:57 PDT
Thanks for the review. > somewhere where all are covered? Probably in finishCreation. The other two constructors respectively use a null prototype and copy the prototype of another struct. So I don't think covering them would bring anything. > Probably also worth doing in Structure::changePrototypeTransition Will do.
Saam Barati
Comment 17 2019-04-08 16:20:29 PDT
(In reply to Robin Morisset from comment #16) > Thanks for the review. > > > somewhere where all are covered? Probably in finishCreation. > > The other two constructors respectively use a null prototype and copy the > prototype of another struct. So I don't think covering them would bring > anything. I agree that this is good enough for the state of the world today, but it's better to put these kinds of assertions in a place where we won't miss it when adding new constructors to Structure in the future. > > > Probably also worth doing in Structure::changePrototypeTransition > > Will do.
Robin Morisset
Comment 18 2019-04-09 11:13:16 PDT
Created attachment 367056 [details] Patch Applied Saam's suggestions, and fixed one more missing didBecomePrototype, found by EWS. Waiting for EWS to confirm this was the last one before landing.
Robin Morisset
Comment 19 2019-04-09 11:18:30 PDT
Created attachment 367057 [details] Patch Missing file and forgotten OOPS in changelog.
EWS Watchlist
Comment 20 2019-04-09 11:20:06 PDT
Attachment 367057 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
EWS Watchlist
Comment 21 2019-04-09 12:27:45 PDT
Comment on attachment 367057 [details] Patch Attachment 367057 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11820335 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 22 2019-04-09 12:27:53 PDT
Created attachment 367061 [details] Archive of layout-test-results from ews116 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-highsierra Platform: Mac OS X 10.13.6
Robin Morisset
Comment 23 2019-04-09 16:40:42 PDT
Created attachment 367081 [details] Patch Yet another missing didBecomePrototype, this one in webcore bindings generated by a perl script. Sending it for another round of EWS.
EWS Watchlist
Comment 24 2019-04-09 16:44:47 PDT
Comment on attachment 367081 [details] Patch Attachment 367081 [details] did not pass bindings-ews (mac): Output: https://webkit-queues.webkit.org/results/11823194 New failing tests: (JS) JSTestCallTracer.cpp (JS) JSTestCEReactions.cpp (JS) JSTestCEReactionsStringifier.cpp (JS) JSTestClassWithJSBuiltinConstructor.cpp (JS) JSTestActiveDOMObject.cpp (JS) JSTestDOMJIT.cpp (JS) JSTestEnabledBySetting.cpp (JS) JSTestEventConstructor.cpp (JS) JSTestEventTarget.cpp (JS) JSTestException.cpp (JS) JSTestGenerateIsReachable.cpp (JS) JSTestGlobalObject.h (JS) JSTestIndexedSetterNoIdentifier.cpp (JS) JSTestIndexedSetterThrowingException.cpp (JS) JSTestIndexedSetterWithIdentifier.cpp (JS) JSTestInterface.cpp (JS) JSTestInterfaceLeadingUnderscore.cpp (JS) JSTestIterable.cpp (JS) JSTestJSBuiltinConstructor.cpp (JS) JSMapLike.cpp (JS) JSTestMediaQueryListListener.cpp (JS) JSTestNamedAndIndexedSetterNoIdentifier.cpp (JS) JSTestNamedAndIndexedSetterThrowingException.cpp (JS) JSTestNamedAndIndexedSetterWithIdentifier.cpp (JS) JSTestNamedConstructor.cpp (JS) JSTestNamedDeleterNoIdentifier.cpp (JS) JSTestNamedDeleterThrowingException.cpp (JS) JSTestNamedDeleterWithIdentifier.cpp (JS) JSTestNamedDeleterWithIndexedGetter.cpp (JS) JSTestNamedGetterCallWith.cpp (JS) JSTestNamedGetterNoIdentifier.cpp (JS) JSTestNamedGetterWithIdentifier.cpp (JS) JSTestNamedSetterNoIdentifier.cpp (JS) JSTestNamedSetterThrowingException.cpp (JS) JSTestNamedSetterWithIdentifier.cpp (JS) JSTestNamedSetterWithIndexedGetter.cpp (JS) JSTestNamedSetterWithIndexedGetterAndSetter.cpp (JS) JSTestNamedSetterWithOverrideBuiltins.cpp (JS) JSTestNamedSetterWithUnforgableProperties.cpp (JS) JSTestNamedSetterWithUnforgablePropertiesAndOverrideBuiltins.cpp (JS) JSTestNode.cpp (JS) JSTestObj.cpp (JS) JSTestOverloadedConstructors.cpp (JS) JSTestOverloadedConstructorsWithSequence.cpp (JS) JSTestOverrideBuiltins.cpp (JS) JSTestPluginInterface.cpp (JS) JSTestPromiseRejectionEvent.cpp (JS) JSReadOnlyMapLike.cpp (JS) JSInterfaceName.cpp (JS) JSTestSerialization.cpp (JS) JSTestSerializationIndirectInheritance.cpp (JS) JSTestSerializationInherit.cpp (JS) JSTestSerializationInheritFinal.cpp (JS) JSTestSerializedScriptValueInterface.cpp (JS) JSTestStringifier.cpp (JS) JSTestStringifierAnonymousOperation.cpp (JS) JSTestStringifierNamedOperation.cpp (JS) JSTestStringifierOperationImplementedAs.cpp (JS) JSTestStringifierOperationNamedToString.cpp (JS) JSTestStringifierReadOnlyAttribute.cpp (JS) JSTestStringifierReadWriteAttribute.cpp (JS) JSTestTypedefs.cpp
Robin Morisset
Comment 25 2019-04-09 17:43:35 PDT
Created attachment 367089 [details] Patch Updating the binding tests that otherwise failed.
EWS Watchlist
Comment 26 2019-04-09 19:19:20 PDT
Comment on attachment 367089 [details] Patch Attachment 367089 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11824624 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 27 2019-04-09 19:19:22 PDT
Created attachment 367097 [details] Archive of layout-test-results from ews114 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Robin Morisset
Comment 28 2019-04-10 10:59:42 PDT
Created attachment 367140 [details] Patch Trying to fix yet another missing didBecomePrototype in code generated by CodeGenerator.pm, sending (again) to EWS for verification.
Robin Morisset
Comment 29 2019-04-12 17:54:49 PDT
EWS Watchlist
Comment 30 2019-04-12 18:00:18 PDT
Comment on attachment 367363 [details] Patch Attachment 367363 [details] did not pass bindings-ews (mac): Output: https://webkit-queues.webkit.org/results/11859081 New failing tests: (JS) JSTestDOMJIT.cpp (JS) JSTestEventConstructor.cpp (JS) JSTestEventTarget.cpp (JS) JSTestNode.cpp (JS) JSTestPromiseRejectionEvent.cpp (JS) JSTestSerializationIndirectInheritance.cpp (JS) JSTestSerializationInherit.cpp (JS) JSTestSerializationInheritFinal.cpp
EWS Watchlist
Comment 31 2019-04-12 18:59:55 PDT
Comment on attachment 367363 [details] Patch Attachment 367363 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11859270 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 32 2019-04-12 18:59:57 PDT
Created attachment 367369 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Robin Morisset
Comment 33 2019-04-15 10:42:52 PDT
Created attachment 367423 [details] Patch Fix another missing didBecomePrototype (this one in JSGlobalObject.cpp::createConsoleProperty), and update the bindings tests. Sending for yet another round of EWS testing, maybe this time it will pass?
Ryan Haddad
Comment 34 2019-04-16 09:11:59 PDT
(In reply to Robin Morisset from comment #33) > Created attachment 367423 [details] > Patch > > Fix another missing didBecomePrototype (this one in > JSGlobalObject.cpp::createConsoleProperty), and update the bindings tests. > Sending for yet another round of EWS testing, maybe this time it will pass? Of note: every macOS Debug WK1 EWS bot that tried to process this patch got stuck and failed to continue testing. It may be worth trying to build this locally and run debug WK1 layout tests to see if it is reproducible. https://webkit-queues.webkit.org/patch/367423/mac-debug-ews
Shawn Roberts
Comment 35 2019-04-19 10:07:04 PDT
(In reply to Ryan Haddad from comment #34) > (In reply to Robin Morisset from comment #33) > > Created attachment 367423 [details] > > Patch > > > > Fix another missing didBecomePrototype (this one in > > JSGlobalObject.cpp::createConsoleProperty), and update the bindings tests. > > Sending for yet another round of EWS testing, maybe this time it will pass? > Of note: every macOS Debug WK1 EWS bot that tried to process this patch got > stuck and failed to continue testing. It may be worth trying to build this > locally and run debug WK1 layout tests to see if it is reproducible. > > https://webkit-queues.webkit.org/patch/367423/mac-debug-ews Hi Robin, this is still happening. https://webkit-queues.webkit.org/queue-status/mac-debug-ews/bots/ews117 (ews112-117) (ews116 is offline, not related) The bots will continually stall after processing this patch. I am going to obsolete your patch so they can move forward.
Robin Morisset
Comment 36 2019-04-19 10:30:35 PDT
(In reply to Shawn Roberts from comment #35) > (In reply to Ryan Haddad from comment #34) > > (In reply to Robin Morisset from comment #33) > > > Created attachment 367423 [details] > > > Patch > > > > > > Fix another missing didBecomePrototype (this one in > > > JSGlobalObject.cpp::createConsoleProperty), and update the bindings tests. > > > Sending for yet another round of EWS testing, maybe this time it will pass? > > Of note: every macOS Debug WK1 EWS bot that tried to process this patch got > > stuck and failed to continue testing. It may be worth trying to build this > > locally and run debug WK1 layout tests to see if it is reproducible. > > > > https://webkit-queues.webkit.org/patch/367423/mac-debug-ews > > Hi Robin, this is still happening. > > https://webkit-queues.webkit.org/queue-status/mac-debug-ews/bots/ews117 > (ews112-117) (ews116 is offline, not related) > > The bots will continually stall after processing this patch. I am going to > obsolete your patch so they can move forward. Oh sorry, I thought the bots had automatically given up after failing to process this patch. I agree that it should be obsoleted until I find the time to test it locally and understand what is wrong with it.
Robin Morisset
Comment 37 2019-04-26 14:41:45 PDT
Created attachment 368352 [details] Patch I was able to build this patch without issues on my MacBook Pro, so I have no idea why the bots choked on it. So I rebased it on ToT, and decided to try it again. I will watch the bots more carefully this time, and cancel/obsolete it if they break again. Hopefully it was just a transient failure, or maybe the patch file itself got slightly corrupted.
Shawn Roberts
Comment 38 2019-04-26 14:45:27 PDT
(In reply to Robin Morisset from comment #37) > Created attachment 368352 [details] > Patch > > I was able to build this patch without issues on my MacBook Pro, so I have > no idea why the bots choked on it. > So I rebased it on ToT, and decided to try it again. I will watch the bots > more carefully this time, and cancel/obsolete it if they break again. > Hopefully it was just a transient failure, or maybe the patch file itself > got slightly corrupted. The issues we were having back then were actually unrelated to your patch. The team identified an issue that was causing EWS to hang after processing patches with failures. that issue is resolved now. Any errors you get will be actual, and we should not need to obsolete patches any longer.
Robin Morisset
Comment 39 2019-04-26 14:48:05 PDT
(In reply to Shawn Roberts from comment #38) > (In reply to Robin Morisset from comment #37) > > Created attachment 368352 [details] > > Patch > > > > I was able to build this patch without issues on my MacBook Pro, so I have > > no idea why the bots choked on it. > > So I rebased it on ToT, and decided to try it again. I will watch the bots > > more carefully this time, and cancel/obsolete it if they break again. > > Hopefully it was just a transient failure, or maybe the patch file itself > > got slightly corrupted. > > The issues we were having back then were actually unrelated to your patch. > The team identified an issue that was causing EWS to hang after processing > patches with failures. that issue is resolved now. Any errors you get will > be actual, and we should not need to obsolete patches any longer. Oh ok! I had been trying to debug the patch locally and could not (obviously, with hindsight) find anything wrong with it.
WebKit Commit Bot
Comment 40 2019-04-26 15:21:31 PDT
Comment on attachment 368352 [details] Patch Clearing flags on attachment: 368352 Committed r244708: <https://trac.webkit.org/changeset/244708>
WebKit Commit Bot
Comment 41 2019-04-26 15:21:34 PDT
All reviewed patches have been landed. Closing bug.
Robin Morisset
Comment 42 2019-04-26 16:18:17 PDT
Oops, the commit queue did not wait for the debug bot, and this patch seems to have broken the build in debug mode. Trying to fix it real fast, will ask for a roll-out if I can't fix it by the end of this afternoon.
Robin Morisset
Comment 43 2019-04-26 16:22:30 PDT
The specific error message, which happens for basically every test in LayoutTests is: dyld: Symbol not found: __ZN3JSC10DisallowGC19s_scopeReentryCountE Referenced from: /Users/rmorisset/Webkit/OpenSource/WebKitBuild/Debug/jsc Expected in: /System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore in /Users/rmorisset/Webkit/OpenSource/WebKitBuild/Debug/jsc This is quite surprising for two reasons: I have not touched that code at all, and JSC::DisallowGC::s_scopeReentryCount is already marked as JS_EXPORT_PRIVATE.
WebKit Commit Bot
Comment 44 2019-04-26 17:32:36 PDT
Re-opened since this is blocked by bug 197334
Saam Barati
Comment 45 2019-05-02 12:20:04 PDT
*** Bug 196896 has been marked as a duplicate of this bug. ***
Darin Adler
Comment 46 2019-05-02 14:37:34 PDT
(In reply to Robin Morisset from comment #43) > The specific error message, which happens for basically every test in > LayoutTests is: > dyld: Symbol not found: __ZN3JSC10DisallowGC19s_scopeReentryCountE > Referenced from: /Users/rmorisset/Webkit/OpenSource/WebKitBuild/Debug/jsc > Expected in: > /System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore > in /Users/rmorisset/Webkit/OpenSource/WebKitBuild/Debug/jsc > > This is quite surprising for two reasons: I have not touched that code at > all, and JSC::DisallowGC::s_scopeReentryCount is already marked as > JS_EXPORT_PRIVATE. That’s a symbol missing in the *system* copy of JavaScriptCore, not the just-built version. The issue here is that the jsc tool is loading the system copy of the JavaScriptCore framework, not the one that was built alongside it. It’s not surprising that it needs to run the correct version since it uses internals and inline functions from the framework, and doesn’t use public API or SPI. And also, for testing, clearly we want to run the framework we are trying to test, not a different one. The question is how this is supposed to work. What is supposed to make jsc load the correct version of JavaScriptCore? Perhaps DYLD_FRAMEOWRK_PATH environment variable is supposed to be set? This is what needs to be investigated.
Robin Morisset
Comment 47 2019-05-03 14:01:42 PDT
Created attachment 368983 [details] Patch I was able to fix my local problem, it was just a missing DYLD_FRAMEWORK (run-jsc automatically does it, but visibly not run-layout-jsc). However, I still cannot reproduce the EWS failures, so sending it again for a full round of EWS, maybe this time I will get some useful error messages.
EWS Watchlist
Comment 48 2019-05-03 17:11:29 PDT
Comment on attachment 368983 [details] Patch Attachment 368983 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12093558 New failing tests: legacy-animation-engine/fast/layers/no-clipping-overflow-hidden-hardware-acceleration.html http/tests/css/filters-on-iframes.html
EWS Watchlist
Comment 49 2019-05-03 17:11:32 PDT
Created attachment 369025 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Robin Morisset
Comment 50 2019-05-07 12:31:22 PDT
Comment on attachment 368983 [details] Patch The test failure on windows also happens without my patch, so let's try landing it.
WebKit Commit Bot
Comment 51 2019-05-07 13:59:49 PDT
Comment on attachment 368983 [details] Patch Clearing flags on attachment: 368983 Committed r245031: <https://trac.webkit.org/changeset/245031>
WebKit Commit Bot
Comment 52 2019-05-07 13:59:52 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 53 2019-05-08 12:58:35 PDT
I don’t understand. The patch that landed is just change logs, no code change?
Darin Adler
Comment 54 2019-05-08 12:59:03 PDT
Comment on attachment 368983 [details] Patch This patch is only change logs, no code change.
Robin Morisset
Comment 55 2019-05-08 13:15:04 PDT
(In reply to Darin Adler from comment #53) > I don’t understand. The patch that landed is just change logs, no code > change? Oops, thank you very much for catching this. I obviously made some mistake when sending this patch, and forgot to check it before landing it.
Robin Morisset
Comment 56 2019-05-08 13:28:55 PDT
Created attachment 369414 [details] Patch for landing This time with the actual contents
WebKit Commit Bot
Comment 57 2019-05-08 14:13:46 PDT
The commit-queue encountered the following flaky tests while processing attachment 369414 [details]: fetch/fetch-worker-crash.html bug 187257 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 58 2019-05-08 14:14:46 PDT
Comment on attachment 369414 [details] Patch for landing Clearing flags on attachment: 369414 Committed r245068: <https://trac.webkit.org/changeset/245068>
WebKit Commit Bot
Comment 59 2019-05-08 14:14:48 PDT
All reviewed patches have been landed. Closing bug.
Ryan Haddad
Comment 60 2019-05-08 16:37:06 PDT
Layout tests are exiting early on macOS and iOS debug bots due to the following assertion failure: ASSERTION FAILED: m_prototype.get().isEmpty() || isValidPrototype(m_prototype.get()) /Volumes/Data/slave/mojave-debug/build/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/Structure.h(145) : void JSC::Structure::finishCreation(JSC::VM &) 1 0x211182679 WTFCrash 2 0x1f800728b WTFCrashWithInfo(int, char const*, char const*, int) 3 0x1f81fe6d9 JSC::Structure::finishCreation(JSC::VM&) 4 0x1f81fe4c9 JSC::Structure::create(JSC::VM&, JSC::JSGlobalObject*, JSC::JSValue, JSC::TypeInfo const&, JSC::ClassInfo const*, unsigned char, unsigned int) 5 0x1f94e831a WebCore::JSDOMIterator<WebCore::JSURLSearchParams, WebCore::URLSearchParamsIteratorTraits>::createStructure(JSC::VM&, JSC::JSGlobalObject*, JSC::JSValue) 6 0x1f94e7d5a JSC::Structure* WebCore::getDOMStructure<WebCore::JSDOMIterator<WebCore::JSURLSearchParams, WebCore::URLSearchParamsIteratorTraits> >(JSC::VM&, WebCore::JSDOMGlobalObject&) 7 0x1f94e7c25 JSC::JSValue WebCore::iteratorCreate<WebCore::JSDOMIterator<WebCore::JSURLSearchParams, WebCore::URLSearchParamsIteratorTraits> >(WebCore::JSDOMIterator<WebCore::JSURLSearchParams, WebCore::URLSearchParamsIteratorTraits>::Wrapper&, WebCore::IterationKind) 8 0x1f94e7b62 WebCore::jsURLSearchParamsPrototypeFunctionEntriesCaller(JSC::ExecState*, WebCore::JSURLSearchParams*, JSC::ThrowScope&) 9 0x1f94d4b50 long long WebCore::IDLOperation<WebCore::JSURLSearchParams>::call<&(WebCore::jsURLSearchParamsPrototypeFunctionEntriesCaller(JSC::ExecState*, WebCore::JSURLSearchParams*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*) 10 0x1f94d483c WebCore::jsURLSearchParamsPrototypeFunctionEntries(JSC::ExecState*) 11 0x2f729f60116b 12 0x2116a03e4 llint_entry 13 0x21168d263 vmEntryToJavaScript 14 0x2123190b7 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) 15 0x212318690 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*) 16 0x212645ee5 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) 17 0x2126460a1 JSC::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) 18 0x1fa22efe8 WebCore::JSExecState::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) 19 0x1fa22ed06 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*) 20 0x1fa22f0bd WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ExceptionDetails*) 21 0x1fa8dab21 WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&) 22 0x1fa8d8f88 WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) 23 0x1fad7ef67 WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement&, WTF::TextPosition const&) 24 0x1fad7ed8f WebCore::HTMLScriptRunner::execute(WTF::Ref<WebCore::ScriptElement, WTF::DumbPtrTraits<WebCore::ScriptElement> >&&, WTF::TextPosition const&) 25 0x1fad5bbdb WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() 26 0x1fad5c03d WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode, bool, WebCore::PumpSession&) 27 0x1fad5b3f4 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) 28 0x1fad5ad7d WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) 29 0x1fad5d452 WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution() 30 0x1fad5d83d WebCore::HTMLDocumentParser::notifyFinished(WebCore::PendingScript&) 31 0x1fa8b7427 WebCore::PendingScript::notifyClientFinished() LEAK: 2 WebPageProxy https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r245068%20(2551)/results.html
Ryan Haddad
Comment 61 2019-05-08 17:12:17 PDT
(In reply to Ryan Haddad from comment #60) > Layout tests are exiting early on macOS and iOS debug bots due to the > following assertion failure: > > ASSERTION FAILED: m_prototype.get().isEmpty() || > isValidPrototype(m_prototype.get()) > /Volumes/Data/slave/mojave-debug/build/WebKitBuild/Debug/JavaScriptCore. > framework/PrivateHeaders/Structure.h(145) : void > JSC::Structure::finishCreation(JSC::VM &) It looks like Mac-debug EWS was seeing crashes before this patch was landed. I will go ahead and roll it out to unblock EWS and trunk debug bots.
Ryan Haddad
Comment 62 2019-05-08 17:17:47 PDT
Reverted r245068 for reason: Caused debug layout tests to exit early due to an assertion failure. Committed r245082: <https://trac.webkit.org/changeset/245082>
Robin Morisset
Comment 63 2019-05-10 14:26:57 PDT
Created attachment 369598 [details] Patch Trying again to get EWS to test it, this time not forgetting JSDOMIteratorPrototype.
Robin Morisset
Comment 64 2019-05-10 14:28:35 PDT
Comment on attachment 369598 [details] Patch I visibly got the Changelog wrong, so even if EWS is happy with this patch I will need to fix that before landing it.
EWS Watchlist
Comment 65 2019-05-10 16:48:48 PDT
Comment on attachment 369598 [details] Patch Attachment 369598 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12157027 New failing tests: fast/dom/Window/window-function-name-getter-precedence.html http/tests/security/cross-frame-access-frames.html fast/dom/Window/es52-globals.html http/tests/security/isolatedWorld/all-window-prototypes.html http/tests/security/cross-frame-access-get-override.html http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html http/tests/security/cross-frame-access-name-getter.html http/tests/security/cross-frame-access-custom.html js/dom/activation-proto.html http/tests/security/cross-frame-access-first-time.html fast/loader/window-clearing.html js/dom/Object-defineProperty.html fast/dom/Window/window-function-frame-getter-precedence.html http/tests/security/cross-frame-access-call.html http/tests/security/cross-frame-access-get.html
EWS Watchlist
Comment 66 2019-05-10 16:48:50 PDT
Created attachment 369622 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
EWS Watchlist
Comment 67 2019-05-10 23:26:11 PDT
Comment on attachment 369598 [details] Patch Attachment 369598 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12160045 New failing tests: svg/dom/viewspec-parser-1.html svg/dom/SVGViewSpec-defaults.html svg/dom/viewspec-parser-5.html svg/dom/viewspec-parser-7.html svg/dom/viewspec-parser-2.html svg/dom/viewspec-parser-4.html svg/dom/viewspec-parser-3.html svg/dom/viewspec-parser-6.html svg/custom/js-svg-constructors.svg svg/dom/SVGViewSpec.html
EWS Watchlist
Comment 68 2019-05-10 23:26:14 PDT
Created attachment 369640 [details] Archive of layout-test-results from ews214 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Saam Barati
Comment 69 2019-05-28 11:32:41 PDT
*** Bug 198259 has been marked as a duplicate of this bug. ***
Yusuke Suzuki
Comment 70 2019-06-22 01:00:22 PDT
Start rebaselining.
Yusuke Suzuki
Comment 71 2019-06-22 01:12:43 PDT
I think JSGlobalObject's reset prototype things are missing (maybe).
Anthony Lai
Comment 72 2019-06-22 01:51:46 PDT
If reopened, may I ask whether the previous patch is not completely resolving the bug?
Yusuke Suzuki
Comment 73 2019-06-22 02:06:26 PDT
JSDOMWindowProperties should be a prototype.
Yusuke Suzuki
Comment 74 2019-06-22 02:09:44 PDT
Add ASSERT to Structure::setPrototypeWithoutTransition.
Yusuke Suzuki
Comment 75 2019-06-22 02:16:42 PDT
Created attachment 372674 [details] Patch For EWS
Yusuke Suzuki
Comment 76 2019-06-22 02:27:18 PDT
Created attachment 372675 [details] Patch For EWS
Yusuke Suzuki
Comment 77 2019-06-22 11:46:27 PDT
Truitt Savell
Comment 78 2019-06-24 08:43:53 PDT
It looks like the changes in https://trac.webkit.org/changeset/246714/webkit Broke 7 API tests on Mac debug. Build: https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK2%20%28Tests%29/builds/3224 I was able to reproduce this locally, the 7 API tests fail on 246714 but not 246713
Yusuke Suzuki
Comment 79 2019-06-24 11:38:04 PDT
(In reply to Truitt Savell from comment #78) > It looks like the changes in https://trac.webkit.org/changeset/246714/webkit > Broke 7 API tests on Mac debug. > > Build: > https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK2%20%28Tests%29/ > builds/3224 > > I was able to reproduce this locally, the 7 API tests fail on 246714 but not > 246713 Thanks! I'll land a follow-up patch.
Keith Miller
Comment 80 2019-06-24 18:16:10 PDT
Yusuke, Saam and I talked about this offline and we came to the conclusion we should set the didBecomePrototype bit in Structure::create instead. I'll revert this change for now.
WebKit Commit Bot
Comment 81 2019-06-24 18:17:27 PDT
Re-opened since this is blocked by bug 199179
Ryan Haddad
Comment 82 2019-06-24 20:38:01 PDT
*** Bug 199139 has been marked as a duplicate of this bug. ***
Keith Miller
Comment 83 2019-06-25 11:57:46 PDT
WebKit Commit Bot
Comment 84 2019-06-25 12:49:27 PDT
Comment on attachment 372851 [details] Patch Clearing flags on attachment: 372851 Committed r246801: <https://trac.webkit.org/changeset/246801>
WebKit Commit Bot
Comment 85 2019-06-25 12:49:30 PDT
All reviewed patches have been landed. Closing bug.
Keith Miller
Comment 86 2019-06-25 13:59:38 PDT
Yusuke Suzuki
Comment 87 2020-04-14 09:45:41 PDT
*** Bug 197557 has been marked as a duplicate of this bug. ***
Yusuke Suzuki
Comment 88 2020-06-12 19:05:18 PDT
*** Bug 145763 has been marked as a duplicate of this bug. ***
Brent Fulgham
Comment 89 2022-02-12 19:51:18 PST
*** Bug 196677 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.