943418 - current-inner checking for history is broken

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: 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

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

That said, I can add it explicitly, I guess.

Comment 5

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/200590163c1f

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/200590163c1f