current-inner checking for history is broken

RESOLVED FIXED in mozilla28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla28
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

This is blocking me fixing bug 936056.

In particular, that patch changes us to actually pass the inner as thisval to DOM getters, so it's actually very easy to get a history object with a non-current inner.  Like so:

  <script>
    window.onload = function() { 
      document.open();
      history.length; // throws
   }
  </script>

The key part is that the spec talks about _documents_ but we implemented in terms of _windows_ and in this case the document is active but the window is not.
Created attachment 8338577 [details] [diff] [review]
Check for active documents, not current inners, in History APIs.

I considered adding a separate nsIDocument::IsActiveDocument, but then I never ended up needing it, hence just the comments on how IsActive() is not it
Attachment #8338577 - Flags: review?(bobbyholley+bmo)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Whiteboard: [need review]
Comment on attachment 8338577 [details] [diff] [review]
Check for active documents, not current inners, in History APIs.

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

So, we're sure that this is the behavior that we want here? I'd kind of prefer that we didn't have to do this, but presumably this is enough of a problem for web-compat?

For the document.open case, the spec still associates the old Window with the document, right?

Please make sure this testcase ends up in the tree, either with this bug or the other one.

::: dom/base/nsPIDOMWindow.h
@@ +335,5 @@
>                 "It doesn't make sense to call this on outer windows.");
>      return mOuterWindow && mOuterWindow->GetCurrentInnerWindow() == this;
>    }
>  
> +  bool HasActiveDocument()

Please add a comment here explaining the case that this covers.
Attachment #8338577 - Flags: review?(bobbyholley+bmo) → review+
> For the document.open case, the spec still associates the old Window with the document,
> right?

Yes.

In fact, the document.open case is the only case in which this check differs from the old one.

> Please make sure this testcase ends up in the tree

It already is, effectively; that's why the patch in bug 936056 is orange on try.
That said, I can add it explicitly, I guess.
https://hg.mozilla.org/integration/mozilla-inbound/rev/200590163c1f
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla28
https://hg.mozilla.org/mozilla-central/rev/200590163c1f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.