Bug 68345 - pop-state-event-constructor.html crashes and fails
Summary: pop-state-event-constructor.html crashes and fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-19 02:54 PDT by Kentaro Hara
Modified: 2011-09-26 23:26 PDT (History)
6 users (show)

See Also:


Attachments
Failing test cases (2.04 KB, text/plain)
2011-09-19 02:55 PDT, Kentaro Hara
no flags Details
Patch (22.66 KB, patch)
2011-09-26 00:19 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Just updated ChangeLog (22.79 KB, patch)
2011-09-26 00:33 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
WIP patch to see if the patch passes efl build (24.14 KB, patch)
2011-09-26 01:30 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for review (24.14 KB, patch)
2011-09-26 04:06 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for commit (24.20 KB, patch)
2011-09-26 17:29 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for commit (24.19 KB, patch)
2011-09-26 21:10 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2011-09-19 02:54:43 PDT
I added pop-state-event-constructor.html in this bug (https://bugs.webkit.org/show_bug.cgi?id=67977), but some tests are crashing or failing. 

- The following test cases are failing, as you can see in the attached file. The IDL type of PopStateEvent.state should be 'any' (http://www.whatwg.org/specs/web-apps/current-work/#popstateevent), and the IDL spec of 'any' (http://www.w3.org/TR/WebIDL/#es-any) says that the following test cases should pass.

    FAIL new PopStateEvent('eventType', { state: '' }).state should be undefined (of type undefined). Was  (of type string).
    FAIL new PopStateEvent('eventType', { state: object1 }).state should be [object Object]. Was [object Object].
    FAIL new PopStateEvent('eventType', { state: {valueOf: function () { return object2; } } }).state should be [object Object]. Was [object Object].
    FAIL new PopStateEvent('eventType', { bubbles: true, cancelable: true, state: object3 }).state should be [object Object]. Was [object Object].

- When we pass a DOM object (i.e. an unserializable object) as follows, then DRT crashes. This crash happens only in DRT.

    shouldBe("new PopStateEvent('eventType', { state: document }).state", "document");

The reason for these failures and crash is that PopStateEvent.state is implemented just as SeriazliedScriptValue, and thus it cannot handle ScriptValue. I am preparing a patch for fixing this.
Comment 1 Kentaro Hara 2011-09-19 02:55:31 PDT
Created attachment 107824 [details]
Failing test cases
Comment 2 Kentaro Hara 2011-09-26 00:19:13 PDT
Created attachment 108632 [details]
Patch
Comment 3 Kentaro Hara 2011-09-26 00:33:07 PDT
Created attachment 108633 [details]
Just updated ChangeLog
Comment 4 Gyuyoung Kim 2011-09-26 00:43:50 PDT
Comment on attachment 108633 [details]
Just updated ChangeLog

Attachment 108633 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9843772
Comment 5 Kentaro Hara 2011-09-26 01:30:48 PDT
Created attachment 108645 [details]
WIP patch to see if the patch passes efl build
Comment 6 Kentaro Hara 2011-09-26 04:06:03 PDT
Created attachment 108651 [details]
patch for review
Comment 7 Adam Barth 2011-09-26 11:52:08 PDT
Comment on attachment 108651 [details]
patch for review

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

> Source/WebCore/ChangeLog:17
> +        popped from HistoryItem, but we cannot deserialize the SeriazliedScriptValue into

SeriazliedScriptValue => SerializedScriptValue

> Source/WebCore/ChangeLog:20
> +        PopStateEvent still needs to provide an API to construct it with SeriazliedScriptValue.

SeriazliedScriptValue

> Source/WebCore/bindings/v8/custom/V8PopStateEventCustom.cpp:46
> +    SerializedScriptValue* serializedState = event->serializedState();
> +    return serializedState ? serializedState->deserialize() : event->state().v8Value();

Will this return undefined when the JSC version returns null?

> Source/WebCore/dom/PopStateEvent.cpp:61
> +    , m_state()

We usually omit this declaration when we're just calling the default constructor.
Comment 8 Kentaro Hara 2011-09-26 17:29:59 PDT
Created attachment 108763 [details]
patch for commit
Comment 9 Kentaro Hara 2011-09-26 21:10:39 PDT
Created attachment 108785 [details]
patch for commit
Comment 10 Kentaro Hara 2011-09-26 21:13:06 PDT
Comment on attachment 108651 [details]
patch for review

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

>> Source/WebCore/ChangeLog:17
>> +        popped from HistoryItem, but we cannot deserialize the SeriazliedScriptValue into
> 
> SeriazliedScriptValue => SerializedScriptValue

Thanks! Fixed.

>> Source/WebCore/ChangeLog:20
>> +        PopStateEvent still needs to provide an API to construct it with SeriazliedScriptValue.
> 
> SeriazliedScriptValue

Fixed.

>> Source/WebCore/bindings/v8/custom/V8PopStateEventCustom.cpp:46
>> +    return serializedState ? serializedState->deserialize() : event->state().v8Value();
> 
> Will this return undefined when the JSC version returns null?

Sorry, fixed it.

>> Source/WebCore/dom/PopStateEvent.cpp:61
>> +    , m_state()
> 
> We usually omit this declaration when we're just calling the default constructor.

Omitted.
Comment 11 WebKit Review Bot 2011-09-26 23:26:11 PDT
Comment on attachment 108785 [details]
patch for commit

Clearing flags on attachment: 108785

Committed r96073: <http://trac.webkit.org/changeset/96073>
Comment 12 WebKit Review Bot 2011-09-26 23:26:17 PDT
All reviewed patches have been landed.  Closing bug.