859707 - Convert window.history to new bindings.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Johnny Stenback (:jst)]

window.history should use the new WebIDL based DOM bindings instead of relying on XPConnect as it does right now. I have a patch in the works...

Comment 1

[Johnny Stenback (:jst)]

Attachment: Fix.

This converts nsHistory to use new bindings, and gets rid of nsIDOMHistory all together.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

https://mxr.mozilla.org/addons/search?string=nsIDOMHistory is not exactly a short list ;)

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Looks like most of the use is coming from addon sdk.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 735453 [details] [diff] [review]
Fix.

Review of attachment 735453 [details] [diff] [review]:
-----------------------------------------------------------------

::: addon-sdk/source/lib/sdk/content/content-proxy.js
@@ +624,4 @@
>    // In addition, the first argument has to come from the same compartment.
>    pushState: function (obj) {
>      // Ensure that we are on an object that expose History API
> +    if (!'pushState' in obj) {

That doesn't do what you think it does.

Comment 5

[David Bruant]

(In reply to :Ms2ger from comment #4)
> > +    if (!'pushState' in obj) {
> 
> That doesn't do what you think it does.
Thanks Ms2ger for inspiring me this ECMAScript Regret [1] :-)
JavaScript is annoying sometimes.


[1] https://github.com/DavidBruant/ECMAScript-regrets/issues/27

Comment 6

[Boris Zbarsky [:bzbarsky]]

> +    aRv = NS_ERROR_DOM_SECURITY_ERR;

Prefer aRv.Throw(NS_ERROR_DOM_SECURITY_ERR)

Johnny, would you mind posting a patch with comment 4 and more importantly comment 2 addressed?  nsIDOMHistory can be empty, presumably, but needs to exist for now...

Comment 7

[Johnny Stenback (:jst)]

Attachment: Updated fix.

This is updated to trunk, which meant that content-proxy.js, nor the code that I needed to fix there before, exists any more. So that shouldn't be an issue any more. I also added back nsIDOMHistory for extension backwards compat.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 782188 [details] [diff] [review]
Updated fix.

>+++ b/dom/base/nsHistory.cpp

>+nsHistory::GetState(JSContext* cx, ErrorResult& aRv) const

Hmm.  The order of some of the methods has changed?  Was there a reason for that?

>+    return JSVAL_VOID;

JS::UndefinedValue(), please, and same elsewhere in this patch.

>+    jsval jsData = JSVAL_VOID;

  JS::Rooted<JS::Value> jsData(cx);

and then .address() for the GetAsJSVal and JS_WrapValue calls.

>+    JS_WrapValue(cx, &jsData);

You need to check the return value of JS_WrapValue too, no?

>+nsHistory::Go(const Optional< int32_t >& delta, ErrorResult& aRv)

The IDL for this should be "void go(optional long delta = 0);".  And we should raise a spec issue to that effect.  Then the first arg here (which should remain "aDelta") will just be int32_t.

>+nsHistory::PushState(JSContext* cx, JS::Handle<JS::Value> data,

This no longer checks sAllowPushStatePrefStr.  Should that be removed, or should we be checking it in the binding?

Similar for ReplaceState and sAllowReplaceStatePrefStr.

Also, the arguments here should probably have names starting with 'a', and similar for the other methods this patch is adding.

>+nsHistory::IndexedGetter(uint32_t index, bool &found, nsString& retval,

>+  aRv = session_history->GetEntryAtIndex(index, false,

Something needs to test aRv after this, before we clobber it of sh_entry is not null.  Similar for the aRv set if sh_entry is non-null and uri ends up non-null.

This method needs to set "found" somewhere.  Do we not have tests for indexed access on a history object?

>+nsHistory::Length()
>+  if (!nsContentUtils::IsCallerChrome()) {

That's a behavior change, and one that doesn't look web-compatible to me.  Please revert to the current behavior here.

>+++ b/dom/base/nsStructuredCloneContainer.cpp
>+  JS::Rooted<JS::Value> jsData(aCx);
>+  jsData = aData;

  JS::Rooted<JS::Value> jsData(aCx, aData);

>+++ b/dom/webidl/History.webidl
>+  getter DOMString? item(unsigned long index);

We should file a followup to get rid of the [n] bit if we can, since it's web-detectable (e.g. through Object.freeze failing) even if the getter itself is chromeonly.

r=me with the above fixed

Comment 9

[Johnny Stenback (:jst)]

> >+++ b/dom/base/nsHistory.cpp
> 
> >+nsHistory::GetState(JSContext* cx, ErrorResult& aRv) const
> 
> Hmm.  The order of some of the methods has changed?  Was there a reason for
> that?

Not a particularly strong one, but I reordered them to match the order in the WebIDL file.

[...]
> >+nsHistory::PushState(JSContext* cx, JS::Handle<JS::Value> data,
> 
> This no longer checks sAllowPushStatePrefStr.  Should that be removed, or
> should we be checking it in the binding?
> 
> Similar for ReplaceState and sAllowReplaceStatePrefStr.

They're both checked in PushOrReplaceState() where the logic for these methods live.

> Also, the arguments here should probably have names starting with 'a', and
> similar for the other methods this patch is adding.

Fixed. We should make the example codegen follow this convention too, IMO.

[...]
> >+nsHistory::Length()
> >+  if (!nsContentUtils::IsCallerChrome()) {
> 
> That's a behavior change, and one that doesn't look web-compatible to me. 
> Please revert to the current behavior here.

This is the ChromeOnly length getter, so shouldn't affect web-compat AFAIK. GetLength() follows what the old code did.

[...]
> We should file a followup to get rid of the [n] bit if we can, since it's
> web-detectable (e.g. through Object.freeze failing) even if the getter
> itself is chromeonly.

Hmm, interesting... and yeah. From a quick grep here it looks like there's only a handful of users of history[n]. I filed bug 903311.

> r=me with the above fixed

Cool, I'll attach a diff with my fixes that you can look over once you're back...

Comment 10

[Johnny Stenback (:jst)]

Attachment: diff

Fixes for bz's review comments.

Comment 11

[Johnny Stenback (:jst)]

Attachment: Full fix, updated.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 788005 [details] [diff] [review]
diff

>+    if (!aRv.Failed() && JS_WrapValue(aCx, jsData.address())) {
>+      return jsData;
>+    }

No, this needs to be more like so:

  if (aRv.Failed()) {
    return JS::UndefinedValue();
  }

  if (!JS_WrapValue(aCx, jsData.address())) {
    aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
    return JS::UndefinedValue();
  }

  return jsData;

because otherwise the pending exception from the failed JS_WrapValue will get stuck on aCx, which is bad.

r=me with that fixed.

Comment 13

[Johnny Stenback (:jst)]

Attachment: Updated fix. r=bz

Comment 14

[Johnny Stenback (:jst)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/debcebc0d7ee

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/debcebc0d7ee