Bug 93096

Summary: regression(r124510) webintents/web-intents-obj-constructor.html is crashing
Product: WebKit Reporter: Sudarsana Nagineni (babu) <naginenis>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, fpizlo, gyuyoung.kim, haraken, lucas.de.marchi, oliver, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 93031    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
haraken: review+, haraken: commit-queue-
Patch for landing none

Description Sudarsana Nagineni (babu) 2012-08-03 05:28:52 PDT
After r124510, webintents/web-intents-obj-constructor.html begun to crash on EFL.

http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug/r124510%20%283782%29/webintents/web-intents-obj-constructor-crash-log.txt
Comment 1 Chris Dumez 2012-08-12 05:29:29 PDT
crash log for DumpRenderTree (pid 23071):
STDOUT: <empty>
STDERR: 1   0x7fbddbcd7cc3
STDERR: 2   0x7fbdd2efbcb0
STDERR: 3   0x494404 JSC::Register::jsValue() const
STDERR: 4   0x4954f0 JSC::Register::scopeChain() const
STDERR: 5   0x4944c2 JSC::ExecState::scopeChain() const
STDERR: 6   0x495456 JSC::ExecState::globalData() const
STDERR: 7   0x7fbdd7e5a629 WebCore::JSDictionary::JSDictionary(JSC::ExecState*, JSC::JSObject*)
STDERR: 8   0x7fbdd7e5a2b6 WebCore::Dictionary::Dictionary()
STDERR: 9   0x7fbdd71a9402 WebCore::Intent::create(JSC::ExecState*, WebCore::Dictionary const&, int&)
STDERR: 10  0x7fbdd7f159b0 WebCore::JSIntentConstructor::constructJSIntent(JSC::ExecState*)
STDERR: 11  0x7fbddbb8100d
STDERR: 12  0x7fbddbb7c004
STDERR: 13  0x7fff68fe9910
STDERR: LEAK: 2 JSLazyEventListener
STDERR: LEAK: 21 RenderObject
STDERR: LEAK: 1 Page
STDERR: LEAK: 1 Frame
STDERR: LEAK: 5 CachedResource
STDERR: LEAK: 1 SubresourceLoader
STDERR: LEAK: 215 WebCoreNode
Comment 2 Chris Dumez 2012-08-12 05:42:12 PDT
Due to r124510, we can no longer pass 0 for JSC::ExecState* argument of:
WebCore::JSDictionary::JSDictionary(JSC::ExecState*, JSC::JSObject*)

This is because, the exec argument is now used in the constructor without NULL check.

We pass 0 JSC::ExecState* argument, when using the Dictionary default constructor, e.g.:
Dictionary d; // Will call WebCore::JSDictionary::JSDictionary(0, 0) internally
Comment 3 Chris Dumez 2012-08-12 07:15:35 PDT
Created attachment 157898 [details]
Patch
Comment 4 Chris Dumez 2012-08-12 07:17:19 PDT
Created attachment 157899 [details]
Patch

Minor typo fix.
Comment 5 Chris Dumez 2012-08-13 08:27:51 PDT
Created attachment 158006 [details]
Patch

Update bug title
Comment 6 Kentaro Hara 2012-08-13 16:43:12 PDT
Comment on attachment 158006 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        before using it. The exec may indeed be null, thus causing crashes.

Who passes a null? I've expected that we always pass a valid exec to the JSDictionary constructor.
Comment 7 Kentaro Hara 2012-08-13 16:44:08 PDT
(In reply to comment #6)
> Who passes a null? I've expected that we always pass a valid exec to the JSDictionary constructor.

Sorry, now I read your comment #2:)
Comment 8 Kentaro Hara 2012-08-13 16:57:50 PDT
Comment on attachment 158006 [details]
Patch

Shall we insert ASSERT(isValid()) to JSDictionary::getWithUndefinedOrNullCheck() and JSDictionary::tryGetProperty() so that we can prevent a null exec and a null initializerObject from being used?
Comment 9 Chris Dumez 2012-08-13 22:35:19 PDT
(In reply to comment #8)
> (From update of attachment 158006 [details])
> Shall we insert ASSERT(isValid()) to JSDictionary::getWithUndefinedOrNullCheck() and JSDictionary::tryGetProperty() so that we can prevent a null exec and a null initializerObject from being used?

Agreed. I'll add those assertions before landing, thanks.
Comment 10 Chris Dumez 2012-08-13 23:27:25 PDT
Created attachment 158232 [details]
Patch for landing

Add assertions as advised by haraken.
Comment 11 WebKit Review Bot 2012-08-14 00:22:54 PDT
Comment on attachment 158232 [details]
Patch for landing

Clearing flags on attachment: 158232

Committed r125513: <http://trac.webkit.org/changeset/125513>
Comment 12 WebKit Review Bot 2012-08-14 00:23:00 PDT
All reviewed patches have been landed.  Closing bug.