Bug 196315 - Structure::create should call didBecomePrototype()
Summary: Structure::create should call didBecomePrototype()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
: 145763 196677 196896 197557 198259 199139 (view as bug list)
Depends on: 197334 199179
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-27 13:48 PDT by Robin Morisset
Modified: 2022-02-12 19:51 PST (History)
18 users (show)

See Also:


Attachments
Patch (19.04 KB, patch)
2019-03-27 13:53 PDT, Robin Morisset
ysuzuki: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
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 Details
Patch (26.07 KB, patch)
2019-03-27 18:08 PDT, Robin Morisset
saam: review-
Details | Formatted Diff | Diff
Patch (26.01 KB, patch)
2019-04-05 16:03 PDT, Robin Morisset
saam: review+
Details | Formatted Diff | Diff
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 Details
Patch (28.63 KB, patch)
2019-04-09 11:13 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (29.04 KB, patch)
2019-04-09 11:18 PDT, Robin Morisset
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
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 Details
Patch (29.81 KB, patch)
2019-04-09 16:40 PDT, Robin Morisset
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Patch (67.38 KB, patch)
2019-04-09 17:43 PDT, Robin Morisset
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
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 Details
Patch (68.09 KB, patch)
2019-04-10 10:59 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (63.92 KB, patch)
2019-04-12 17:54 PDT, Robin Morisset
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
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 Details
Patch (68.59 KB, patch)
2019-04-15 10:42 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (68.48 KB, patch)
2019-04-26 14:41 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (5.51 KB, patch)
2019-05-03 14:01 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
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 Details
Patch for landing (79.47 KB, patch)
2019-05-08 13:28 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (111.61 KB, patch)
2019-05-10 14:26 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
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 Details
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 Details
Patch (112.47 KB, patch)
2019-06-22 02:16 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (97.24 KB, patch)
2019-06-22 02:27 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (5.98 KB, patch)
2019-06-25 11:57 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 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.
Comment 1 Robin Morisset 2019-03-27 13:49:34 PDT
rdar://problem/49182141
Comment 2 Robin Morisset 2019-03-27 13:53:38 PDT
Created attachment 366101 [details]
Patch
Comment 3 Yusuke Suzuki 2019-03-27 13:58:09 PDT
Comment on attachment 366101 [details]
Patch

Nice, r=me
Comment 4 EWS Watchlist 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.
Comment 5 EWS Watchlist 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
Comment 6 Robin Morisset 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?)
Comment 7 Saam Barati 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
Comment 8 Saam Barati 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.
Comment 9 Saam Barati 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
Comment 10 Saam Barati 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());
    }
}
Comment 11 Saam Barati 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.
Comment 12 Robin Morisset 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.
Comment 13 EWS Watchlist 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.
Comment 14 EWS Watchlist 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
Comment 15 Saam Barati 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
Comment 16 Robin Morisset 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.
Comment 17 Saam Barati 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.
Comment 18 Robin Morisset 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.
Comment 19 Robin Morisset 2019-04-09 11:18:30 PDT
Created attachment 367057 [details]
Patch

Missing file and forgotten OOPS in changelog.
Comment 20 EWS Watchlist 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.
Comment 21 EWS Watchlist 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.
Comment 22 EWS Watchlist 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
Comment 23 Robin Morisset 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.
Comment 24 EWS Watchlist 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
Comment 25 Robin Morisset 2019-04-09 17:43:35 PDT
Created attachment 367089 [details]
Patch

Updating the binding tests that otherwise failed.
Comment 26 EWS Watchlist 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.
Comment 27 EWS Watchlist 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
Comment 28 Robin Morisset 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.
Comment 29 Robin Morisset 2019-04-12 17:54:49 PDT
Created attachment 367363 [details]
Patch
Comment 30 EWS Watchlist 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
Comment 31 EWS Watchlist 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.
Comment 32 EWS Watchlist 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
Comment 33 Robin Morisset 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?
Comment 34 Ryan Haddad 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
Comment 35 Shawn Roberts 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.
Comment 36 Robin Morisset 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.
Comment 37 Robin Morisset 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.
Comment 38 Shawn Roberts 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.
Comment 39 Robin Morisset 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.
Comment 40 WebKit Commit Bot 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>
Comment 41 WebKit Commit Bot 2019-04-26 15:21:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 42 Robin Morisset 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.
Comment 43 Robin Morisset 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.
Comment 44 WebKit Commit Bot 2019-04-26 17:32:36 PDT
Re-opened since this is blocked by bug 197334
Comment 45 Saam Barati 2019-05-02 12:20:04 PDT
*** Bug 196896 has been marked as a duplicate of this bug. ***
Comment 46 Darin Adler 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.
Comment 47 Robin Morisset 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.
Comment 48 EWS Watchlist 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
Comment 49 EWS Watchlist 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
Comment 50 Robin Morisset 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.
Comment 51 WebKit Commit Bot 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>
Comment 52 WebKit Commit Bot 2019-05-07 13:59:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 53 Darin Adler 2019-05-08 12:58:35 PDT
I don’t understand. The patch that landed is just change logs, no code change?
Comment 54 Darin Adler 2019-05-08 12:59:03 PDT
Comment on attachment 368983 [details]
Patch

This patch is only change logs, no code change.
Comment 55 Robin Morisset 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.
Comment 56 Robin Morisset 2019-05-08 13:28:55 PDT
Created attachment 369414 [details]
Patch for landing

This time with the actual contents
Comment 57 WebKit Commit Bot 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.
Comment 58 WebKit Commit Bot 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>
Comment 59 WebKit Commit Bot 2019-05-08 14:14:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 60 Ryan Haddad 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
Comment 61 Ryan Haddad 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.
Comment 62 Ryan Haddad 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>
Comment 63 Robin Morisset 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.
Comment 64 Robin Morisset 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.
Comment 65 EWS Watchlist 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
Comment 66 EWS Watchlist 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
Comment 67 EWS Watchlist 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
Comment 68 EWS Watchlist 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
Comment 69 Saam Barati 2019-05-28 11:32:41 PDT
*** Bug 198259 has been marked as a duplicate of this bug. ***
Comment 70 Yusuke Suzuki 2019-06-22 01:00:22 PDT
Start rebaselining.
Comment 71 Yusuke Suzuki 2019-06-22 01:12:43 PDT
I think JSGlobalObject's reset prototype things are missing (maybe).
Comment 72 Anthony Lai 2019-06-22 01:51:46 PDT
If reopened, may I ask whether the previous patch is not completely resolving the bug?
Comment 73 Yusuke Suzuki 2019-06-22 02:06:26 PDT
JSDOMWindowProperties should be a prototype.
Comment 74 Yusuke Suzuki 2019-06-22 02:09:44 PDT
Add ASSERT to Structure::setPrototypeWithoutTransition.
Comment 75 Yusuke Suzuki 2019-06-22 02:16:42 PDT
Created attachment 372674 [details]
Patch

For EWS
Comment 76 Yusuke Suzuki 2019-06-22 02:27:18 PDT
Created attachment 372675 [details]
Patch

For EWS
Comment 77 Yusuke Suzuki 2019-06-22 11:46:27 PDT
Committed r246714: <https://trac.webkit.org/changeset/246714>
Comment 78 Truitt Savell 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
Comment 79 Yusuke Suzuki 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.
Comment 80 Keith Miller 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.
Comment 81 WebKit Commit Bot 2019-06-24 18:17:27 PDT
Re-opened since this is blocked by bug 199179
Comment 82 Ryan Haddad 2019-06-24 20:38:01 PDT
*** Bug 199139 has been marked as a duplicate of this bug. ***
Comment 83 Keith Miller 2019-06-25 11:57:46 PDT
Created attachment 372851 [details]
Patch
Comment 84 WebKit Commit Bot 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>
Comment 85 WebKit Commit Bot 2019-06-25 12:49:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 86 Keith Miller 2019-06-25 13:59:38 PDT
Debug bug fixes in: https://bugs.webkit.org/show_bug.cgi?id=199202.
Comment 87 Yusuke Suzuki 2020-04-14 09:45:41 PDT
*** Bug 197557 has been marked as a duplicate of this bug. ***
Comment 88 Yusuke Suzuki 2020-06-12 19:05:18 PDT
*** Bug 145763 has been marked as a duplicate of this bug. ***
Comment 89 Brent Fulgham 2022-02-12 19:51:18 PST
*** Bug 196677 has been marked as a duplicate of this bug. ***