Bug 77493 – PopStateEvent.state should use the same object as history.state

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Pablo Flouret]

When state is added to history's dom interface, PopStateEvent should use that same deserialization for the state attribute.

[Pablo Flouret]

Created attachment 126201 [details]
Patch

[Adam Barth]

Comment on attachment 126201 [details]
Patch

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

> Source/WebCore/bindings/js/JSPopStateEventCustom.cpp:53
> +    return jsHistory->get(exec, Identifier(exec, "state"));

If the web page installs a getter on the history object, does this call that getter?  (Seems like something we should test.)

> Source/WebCore/bindings/v8/custom/V8PopStateEventCustom.cpp:55
> +    return toV8(history).As<v8::Object>()->Get(v8::String::NewSymbol("state"));

I would think you'd want to get the hidden property rather than the real property.  The hidden property can't be manipulated by the web page where as the real property can.

[Pablo Flouret]

Yeah, and i found other issues as well, so i'll make more tests and maybe rethink it a bit.

[Pablo Flouret]

It seems the existing behavior for PopStateEvent.state is not totally correct, or at least different to the other browsers, due to history.back et al being asynchronous.

Consider:

<script>
history.pushState(1, "", "");                                   
history.pushState(2, "", "");                                   
window.onpopstate = function (e) { alert(e.state); }
history.back();
history.pushState(3, "", "");
</script>

Webkit alerts 2, gecko alerts 1, presto doesn't even fire the event (pushState(3) cancels the navigation?)

I'll try to find a better solution to the whole thing and spam some lists in the meantime to see what should happen here. In the meantime ignore the patch, it's very wrong.

[Pablo Flouret]

Created attachment 126594 [details]
Patch

New try. The bindings code is a bit fragile, i wonder if the first approach in 76035 (keeping the ScriptValue in HistoryItem) wasn't cleaner after all.

[Pablo Flouret]

(In reply to comment #2)
> (From update of attachment 126201 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126201&action=review
> 
> > Source/WebCore/bindings/js/JSPopStateEventCustom.cpp:53
> > +    return jsHistory->get(exec, Identifier(exec, "state"));
> 
> If the web page installs a getter on the history object, does this call that getter?  (Seems like something we should test.)
> 

Apparently the attribute in JSC isn't configurable so there would be no way to set a custom getter, right? I modified it to call jsHistory->state() directly to be sure.

[Brady Eidson]

(In reply to comment #6)
> (In reply to comment #2)
> > (From update of attachment 126201 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=126201&action=review
> > 
> > > Source/WebCore/bindings/js/JSPopStateEventCustom.cpp:53
> > > +    return jsHistory->get(exec, Identifier(exec, "state"));
> > 
> > If the web page installs a getter on the history object, does this call that getter?  (Seems like something we should test.)
> > 
> 
> Apparently the attribute in JSC isn't configurable so there would be no way to set a custom getter, right? I modified it to call jsHistory->state() directly to be sure.

The History IDL says history.state is readonly.  I don't think a page can install a custom getter.

[Pablo Flouret]

Ping?

[Adam Barth]

Comment on attachment 126594 [details]
Patch

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

This feels more complicated than it should be, but I don't immediately see a way to simplify it.  Can we try for a version that doesn't use goto?

> Source/WebCore/bindings/js/JSPopStateEventCustom.cpp:50
> +    History* history;
> +    bool isSameState;

Generally we like to declare variables at the same time as they're initialized.  Here, we should move the History* history declaration to line 57

> Source/WebCore/bindings/js/JSPopStateEventCustom.cpp:54
> +        goto saveResultAndReturnIt;

There's got to be a better way to structure this code than to use goto.  Perhaps use a helper function that returns the proper result?

[Pablo Flouret]

Created attachment 127283 [details]
Got rid of gotos

[Pablo Flouret]

Ping again? :-)

[Adam Barth]

Comment on attachment 127283 [details]
Got rid of gotos

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

> Source/WebCore/bindings/v8/custom/V8PopStateEventCustom.cpp:53
> +    v8::Handle<v8::String> propertyName = v8::String::NewSymbol("state");

Why not use V8HiddenPropertyNames ?

[Adam Barth]

Sorry for the delay in reviewing.  One nit above.

[Pablo Flouret]

(In reply to comment #12)
> (From update of attachment 127283 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127283&action=review
> 
> > Source/WebCore/bindings/v8/custom/V8PopStateEventCustom.cpp:53
> > +    v8::Handle<v8::String> propertyName = v8::String::NewSymbol("state");
> 
> Why not use V8HiddenPropertyNames ?

Wasn't aware of its existence, will use, thanks.

[Pablo Flouret]

Created attachment 128324 [details]
Use V8HiddenPropertyName

[Kentaro Hara]

Comment on attachment 128324 [details]
Use V8HiddenPropertyName

One good way not to invalidate the previous r+ is as follows:
- Manually change the "Reviewed by NOBODY" line to "Reviewed by Adam Barth"
- Upload the patch by './webkit-patch upload ... --no-review --no-obsolete --request-commit -m "patch for landing"'

[WebKit Review Bot]

Comment on attachment 128324 [details]
Use V8HiddenPropertyName

Clearing flags on attachment: 128324

Committed r108596: <http://trac.webkit.org/changeset/108596>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.