Bug 77493 - PopStateEvent.state should use the same object as history.state
Summary: PopStateEvent.state should use the same object as history.state
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: 76035
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-31 16:30 PST by Pablo Flouret
Modified: 2012-02-22 19:05 PST (History)
7 users (show)

See Also:


Attachments
Patch (17.55 KB, patch)
2012-02-08 17:30 PST, Pablo Flouret
abarth: review-
Details | Formatted Diff | Diff
Patch (20.49 KB, patch)
2012-02-10 15:31 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
Got rid of gotos (21.01 KB, patch)
2012-02-15 17:36 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
Use V8HiddenPropertyName (23.95 KB, patch)
2012-02-22 16:24 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pablo Flouret 2012-01-31 16:30:52 PST
When state is added to history's dom interface, PopStateEvent should use that same deserialization for the state attribute.
Comment 1 Pablo Flouret 2012-02-08 17:30:54 PST
Created attachment 126201 [details]
Patch
Comment 2 Adam Barth 2012-02-08 17:37:55 PST
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.
Comment 3 Pablo Flouret 2012-02-08 22:40:42 PST
Yeah, and i found other issues as well, so i'll make more tests and maybe rethink it a bit.
Comment 4 Pablo Flouret 2012-02-09 11:12:20 PST
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.
Comment 5 Pablo Flouret 2012-02-10 15:31:09 PST
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.
Comment 6 Pablo Flouret 2012-02-10 15:33:30 PST
(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.
Comment 7 Brady Eidson 2012-02-10 15:40:05 PST
(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.
Comment 8 Pablo Flouret 2012-02-15 15:27:06 PST
Ping?
Comment 9 Adam Barth 2012-02-15 15:39:38 PST
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?
Comment 10 Pablo Flouret 2012-02-15 17:36:44 PST
Created attachment 127283 [details]
Got rid of gotos
Comment 11 Pablo Flouret 2012-02-22 10:59:15 PST
Ping again? :-)
Comment 12 Adam Barth 2012-02-22 15:11:11 PST
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 ?
Comment 13 Adam Barth 2012-02-22 15:11:27 PST
Sorry for the delay in reviewing.  One nit above.
Comment 14 Pablo Flouret 2012-02-22 15:17:16 PST
(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.
Comment 15 Pablo Flouret 2012-02-22 16:24:09 PST
Created attachment 128324 [details]
Use V8HiddenPropertyName
Comment 16 Kentaro Hara 2012-02-22 16:29:32 PST
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"'
Comment 17 WebKit Review Bot 2012-02-22 19:05:39 PST
Comment on attachment 128324 [details]
Use V8HiddenPropertyName

Clearing flags on attachment: 128324

Committed r108596: <http://trac.webkit.org/changeset/108596>
Comment 18 WebKit Review Bot 2012-02-22 19:05:47 PST
All reviewed patches have been landed.  Closing bug.