Bug 156136 – Regression(r196145): Crash in getOwnPropertyDescriptor on http://www.history.com/shows/vikings

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Regression(r196145): Crash in getOwnPropertyDescriptor on http://www.history.com/shows/vikings:
ASSERTION FAILED: maybeGetterSetter
/Volumes/Data/WebKit/OpenSource/Source/JavaScriptCore/runtime/JSObject.cpp(2859) : bool JSC::JSObject::getOwnPropertyDescriptor(JSC::ExecState *, JSC::PropertyName, JSC::PropertyDescriptor &)
1   0x110ffeded WTFCrash
2   0x110b52342 JSC::JSObject::getOwnPropertyDescriptor(JSC::ExecState*, JSC::PropertyName, JSC::PropertyDescriptor&)
3   0x110ca3875 JSC::objectConstructorGetOwnPropertyDescriptor(JSC::ExecState*, JSC::JSObject*, JSC::Identifier const&)
4   0x110ca3f0d JSC::objectConstructorGetOwnPropertyDescriptor(JSC::ExecState*)
5   0x2a29de801028
6   0x110c35cd3 llint_entry
7   0x110c35d4d llint_entry
8   0x110c35d4d llint_entry
9   0x110c35d4d llint_entry
10  0x110c35d4d llint_entry
11  0x110c2f37e vmEntryToJavaScript
12  0x110a4ecb7 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)

The page crashes when doing the following:
Object.getOwnPropertyDescriptor(window, "indexedDB")

getOwnPropertyDescriptor() expected getDirect() to return a CustomGetterSetter for CustomAccessors but it is not the case for window.indexedDB. The reason is that window.indexedDB is a special property currently, which is not part of the static table but returned by GetOwnPropertySlot() if IndexedDB feature is enabled. This weirdness is because we do not have proper support for [EnabledAtRuntime] properties on Window.

[Chris Dumez]

rdar://problem/25410767

[Chris Dumez]

Created attachment 275474 [details]
WIP Patch for ews

[Build Bot]

Comment on attachment 275474 [details]
WIP Patch for ews

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

New failing tests:
js/cached-window-properties.html
fast/dom/Window/window-postmessage-clone-frames.html

[Build Bot]

Created attachment 275476 [details]
Archive of layout-test-results from ews103 for mac-yosemite

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

[Build Bot]

Comment on attachment 275474 [details]
WIP Patch for ews

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

New failing tests:
js/cached-window-properties.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 275474 [details]
WIP Patch for ews

Attachment 275474 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1088230

New failing tests:
js/cached-window-properties.html
fast/dom/Window/window-postmessage-clone-frames.html

[Build Bot]

Created attachment 275479 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Chris Dumez]

Created attachment 275488 [details]
WIP Patch for ews

[Chris Dumez]

Created attachment 275502 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 275502 [details]
Patch

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

Can we add a bindings test?

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:31
> +    macro(AnimationTimeline) \

It's strange that these things are listed in "common" identifiers table.
Do we really need to add them here?

[Chris Dumez]

(In reply to comment #11)
> Comment on attachment 275502 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275502&action=review
> 
> Can we add a bindings test?

The change is specific to DOMWindow so this makes bindings test coverage difficult.
We have no such coverage at the moment.

> 
> > Source/JavaScriptCore/runtime/CommonIdentifiers.h:31
> > +    macro(AnimationTimeline) \
> 
> It's strange that these things are listed in "common" identifiers table.
> Do we really need to add them here?

My initial implementation was creating those PropertyNames on the fly in JSDOMWindow::finishCreation() as so:
PropertyName propertyString = Identifier::fromString(&vm, "indexedDB");

However, this lead to assertion hits in debug when destroying the PropertyTable:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
ASSERT_WITH_MESSAGE(iterator != atomicStringTable.end(), "The string being removed is atomic in the string table of an other thread!");
0   com.apple.JavaScriptCore      	0x0000000105fc4b47 WTFCrash + 39 (Assertions.cpp:322)
1   com.apple.JavaScriptCore      	0x0000000105fc819b WTF::AtomicStringImpl::remove(WTF::AtomicStringImpl*) + 267 (AtomicStringImpl.cpp:453)
2   com.apple.JavaScriptCore      	0x0000000106019643 WTF::StringImpl::~StringImpl() + 163 (StringImpl.cpp:117)
3   com.apple.JavaScriptCore      	0x00000001060197b5 WTF::StringImpl::~StringImpl() + 21 (StringImpl.cpp:134)
4   com.apple.JavaScriptCore      	0x00000001060197d5 WTF::StringImpl::destroy(WTF::StringImpl*) + 21 (StringImpl.cpp:139)
5   com.apple.JavaScriptCore      	0x000000010505b654 WTF::StringImpl::deref() + 52 (StringImpl.h:601)
6   com.apple.JavaScriptCore      	0x0000000105d95ed1 JSC::PropertyTable::~PropertyTable() + 97 (PropertyTable.cpp:127)
7   com.apple.JavaScriptCore      	0x0000000105d95e65 JSC::PropertyTable::~PropertyTable() + 21 (PropertyTable.cpp:130)
8   com.apple.JavaScriptCore      	0x0000000105d94fe5 JSC::PropertyTable::destroy(JSC::JSCell*) + 21 (PropertyTable.cpp:121)
9   com.apple.JavaScriptCore      	0x0000000105bfcc98 JSC::MarkedBlock::callDestructor(JSC::JSCell*) + 200 (MarkedBlock.cpp:85)
10  com.apple.JavaScriptCore      	0x0000000105bfcafb JSC::MarkedBlock::FreeList JSC::MarkedBlock::specializedSweep<(JSC::MarkedBlock::BlockState)3, (JSC::MarkedBlock::SweepMode)0, true>() + 267 (MarkedBlock.cpp:102)

I did not understand why this would happen. Maybe a JSC expert can shade some light on this?

[WebKit Commit Bot]

Comment on attachment 275502 [details]
Patch

Clearing flags on attachment: 275502

Committed r199017: <http://trac.webkit.org/changeset/199017>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

This layout test has been timing out on GTK since it was added, see bug #158925.

[Michael Catanzaro]

(In reply to comment #15)
> This layout test has been timing out on GTK since it was added, see bug
> #158925.

Sorry, ignore that, I had my canned reply at the top of my head. Rather, this change appears to have broken the preexisting test media/media-source/media-source-loadedmetada-with-two-sourcebuffers.html. See bug #158925.