Convert window.history to new bindings.

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: jst, Assigned: jst)

Tracking

(Blocks 1 bug)

Trunk
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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...
Posted patch Fix. (obsolete) — Splinter Review
This converts nsHistory to use new bindings, and gets rid of nsIDOMHistory all together.
Attachment #735453 - Flags: review?(peterv)
Looks like most of the use is coming from addon sdk.
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.
(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
> +    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...
Flags: needinfo?(jst)
Posted patch Updated fix. (obsolete) — Splinter Review
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.
Attachment #735453 - Attachment is obsolete: true
Attachment #735453 - Flags: review?(peterv)
Attachment #782188 - Flags: review?(peterv)
Flags: needinfo?(jst)
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
Attachment #782188 - Flags: review?(peterv) → review+
> >+++ 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...
Posted patch diff (obsolete) — Splinter Review
Fixes for bz's review comments.
Attachment #788005 - Flags: review?(bzbarsky)
Posted patch Full fix, updated. (obsolete) — Splinter Review
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.
Attachment #788005 - Flags: review?(bzbarsky) → review+
Attachment #782188 - Attachment is obsolete: true
Attachment #788005 - Attachment is obsolete: true
Attachment #788007 - Attachment is obsolete: true
Attachment #788489 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/debcebc0d7ee
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 923250
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.