Bug 183256 – We need to clear cached structures when having a bad time

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

[Saam Barati]

...

[Saam Barati]

Created attachment 334848 [details]
patch

[Saam Barati]

<rdar://problem/36245022>

[Mark Lam]

Comment on attachment 334848 [details]
patch

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

r=me

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1200
> +    auto isInEffectedGlobalObject = [&] (JSObject* object) {
> +        for (JSObject* current = object; ;) {
> +            if (current->globalObject() == m_globalObject)
> +                return true;
> +            
> +            JSValue prototypeValue = current->getPrototypeDirect(vm);
> +            if (prototypeValue.isNull())
> +                return false;
> +            current = asObject(prototypeValue);
> +        }
> +    };

I think you need a "return false" after the for loop in order to be equivalent to the original code.  But in this case, I think we should never get there.  So, let's have a RELEASE_ASSERT_NOT_REACHED() there.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1211
> +                    bool isRelevantGlobalObject = structure->globalObject() == m_globalObject

I suggest putting parens around the == expr.

[Saam Barati]

Comment on attachment 334848 [details]
patch

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

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1211
>> +                    bool isRelevantGlobalObject = structure->globalObject() == m_globalObject
> 
> I suggest putting parens around the == expr.

For stylistic reasons?

[Mark Lam]

Comment on attachment 334848 [details]
patch

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

>>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1211
>>> +                    bool isRelevantGlobalObject = structure->globalObject() == m_globalObject
>> 
>> I suggest putting parens around the == expr.
> 
> For stylistic reasons?

For clarity, and for not relying on operator precedence to ensure correctness.

[Saam Barati]

(In reply to Mark Lam from comment #5)
> Comment on attachment 334848 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334848&action=review
> 
> >>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1211
> >>> +                    bool isRelevantGlobalObject = structure->globalObject() == m_globalObject
> >> 
> >> I suggest putting parens around the == expr.
> > 
> > For stylistic reasons?
> 
> For clarity, and for not relying on operator precedence to ensure
> correctness.

I’m fine with this change.

However, I don’t think the latter is something we normally do. See: math in most places in WK

[Saam Barati]

Comment on attachment 334848 [details]
patch

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

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1227
> +    if (isInEffectedGlobalObject(object))

This is broken here. Should be inverted.

[Mark Lam]

Comment on attachment 334848 [details]
patch

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

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1227
>> +    if (isInEffectedGlobalObject(object))
> 
> This is broken here. Should be inverted.

Sorry I missed that.

[Saam Barati]

Comment on attachment 334848 [details]
patch

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

>>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1227
>>> +    if (isInEffectedGlobalObject(object))
>> 
>> This is broken here. Should be inverted.
> 
> Sorry I missed that.

No worries. I meant to move the m_foundObjects.append to here, but forgot to. Good thing jsc tests found it.

[Saam Barati]

Created attachment 334856 [details]
patch for landing

[Saam Barati]

Created attachment 334857 [details]
patch for landing

[EWS Watchlist]

Comment on attachment 334857 [details]
patch for landing

Attachment 334857 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6726252

New failing tests:
webrtc/video-replace-muted-track.html

[EWS Watchlist]

Created attachment 334864 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

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

[Saam Barati]

(In reply to Build Bot from comment #12)
> Comment on attachment 334857 [details]
> patch for landing
> 
> Attachment 334857 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.webkit.org/results/6726252
> 
> New failing tests:
> webrtc/video-replace-muted-track.html

Seems unrelated, but it happened twice. Ima build and try to reproduce.

[Saam Barati]

(In reply to Saam Barati from comment #14)
> (In reply to Build Bot from comment #12)
> > Comment on attachment 334857 [details]
> > patch for landing
> > 
> > Attachment 334857 [details] did not pass mac-wk2-ews (mac-wk2):
> > Output: http://webkit-queues.webkit.org/results/6726252
> > 
> > New failing tests:
> > webrtc/video-replace-muted-track.html
> 
> Seems unrelated, but it happened twice. Ima build and try to reproduce.

Reading the test, this seems super unlikely. However, I'll wait until I have a full build to verify.

[Saam Barati]

(In reply to Saam Barati from comment #15)
> (In reply to Saam Barati from comment #14)
> > (In reply to Build Bot from comment #12)
> > > Comment on attachment 334857 [details]
> > > patch for landing
> > > 
> > > Attachment 334857 [details] did not pass mac-wk2-ews (mac-wk2):
> > > Output: http://webkit-queues.webkit.org/results/6726252
> > > 
> > > New failing tests:
> > > webrtc/video-replace-muted-track.html
> > 
> > Seems unrelated, but it happened twice. Ima build and try to reproduce.
> 
> Reading the test, this seems super unlikely. However, I'll wait until I have
> a full build to verify.

This test doesn't "have a bad time" so I have no idea how my change could effect it. I'm going to CQ+.

[WebKit Commit Bot]

Comment on attachment 334857 [details]
patch for landing

Clearing flags on attachment: 334857

Committed r229161: <https://trac.webkit.org/changeset/229161>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.