1265072 - Consider removing nsContentUtils::GetDocumentFromScriptContext()

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

While looking at some EventSource.cpp code we noticed that it was doing things like:

  nsIScriptContext* sc = GetContextForEventHandlers(&rv);
  nsCOMPtr<nsIDocument> doc =
    nsContentUtils::GetDocumentFromScriptContext(sc);

It seems like GetOwner()->GetDoc() may be more appropriate here.

Boris asked me to file this so he could look at it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add GetWindowIfCurrent and GetDocumentIfCurrent helpers to DOMEventTargetHelper

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Get rid of uses of GetDocumentFromScriptContext in XMLHttpRequest code

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Get rid of the use of GetDocumentFromScriptContext in DOMEventTargetHelper

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Get rid of uses of GetDocumentFromScriptContext in WebSocket code

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Get rid of uses of GetDocumentFromScriptContext in EventSource code

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Get rid of uses of GetDocumentFromScriptContext in EventSource code

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Get rid of nsContentUtils::GetDocumentFromScriptContext

Comment 8

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

Comment on attachment 8742580 [details] [diff] [review]
part 1.  Add GetWindowIfCurrent and GetDocumentIfCurrent helpers to DOMEventTargetHelper

>+DOMEventTargetHelper::GetDocumentIfCurrent() const
>+{
>+  nsPIDOMWindowInner* win = GetWindowIfCurrent();
>+  if (!win) {
>+    return nullptr;
>+  }
>+
>+  return win->GetDoc();
Hmm, do we want GetDoc() or GetExtantDoc().
Maybe other patches in this bug will give some hint about it.
In practice it may be rather difficult to get to a state where _inner_ window doesn't have document, yet
a document can be created using GetDoc.

Comment 9

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

Comment on attachment 8742581 [details] [diff] [review]
part 2.  Get rid of uses of GetDocumentFromScriptContext in XMLHttpRequest code

>+  nsCOMPtr<nsIDocument> doc = GetDocumentIfCurrent();
>+  if (!doc) {
>+    // This could be because we're no longer current or because we're in some
>+    // non-window context...
>+    nsresult rv = CheckInnerWindowCorrectness();
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
I wonder if it was odd API if GetDocumentIfCurrent() had an optional nsresult& out param, which would return the result of
innerwindowcorrectness. Or should be even non-optional, to hint that there is something to keep in mind.


>-    nsIScriptContext* sc = GetContextForEventHandlers(&rv);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-    nsCOMPtr<nsIDocument> doc =
>-      nsContentUtils::GetDocumentFromScriptContext(sc);
>+    nsCOMPtr<nsIDocument> doc = GetDocumentIfCurrent();
>     nsCOMPtr<nsIURI> chromeXHRDocURI, chromeXHRDocBaseURI;
>     if (doc) {
>       chromeXHRDocURI = doc->GetDocumentURI();
>       chromeXHRDocBaseURI = doc->GetBaseURI();
>+    } else {
>+      // If we're no longer current, just kill the load, though it really should
>+      // have been killed already.
>+      nsresult rv = CheckInnerWindowCorrectness();
>+      NS_ENSURE_SUCCESS(rv, rv);
>     }
looks like it would help also here.

Comment 10

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

Comment on attachment 8742582 [details] [diff] [review]
part 2.  Get rid of the use of GetDocumentFromScriptContext in DOMEventTargetHelper

such out param would make also this a bit simpler

Comment 11

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

Comment on attachment 8742581 [details] [diff] [review]
part 2.  Get rid of uses of GetDocumentFromScriptContext in XMLHttpRequest code

So I think the out param would make the code less error prone.
The out param would be some error value if we have had inner window, but it isn't the current anymore, but would be success value if we've never had inner window.
If you disagree, ask review again.

Comment 12

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

Comment on attachment 8742585 [details] [diff] [review]
part 5.  Get rid of uses of GetDocumentFromScriptContext in EventSource code

Whatever happens with the out param, this is fine, and thanks for fixing the (unrelated) SetReferrerWithPolicy.
Though, I wonder whether other browsers use GetDocumentURI() or GetOriginalURI()
(I've started to dislike pushState and how it affects to even more than just the fragment part of the url. Too late I guess :))

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Hmm, do we want GetDoc() or GetExtantDoc().

Probably the former.  If the window is the current inner, why would we ever want to not create the document if it doesn't exist already?

> So I think the out param would make the code less error prone.

So when I started I was assuming that consumers just cared about having a current document.  It wasn't until partway through that I realized that some of these objects need to operate in non-document contexts and this throws a monkey wrench into the works.

The problem is, other objects _don't_ need to worry about this (e.g. EventSource, and I think in practice WebSocket; WebSocket::Constructor ensures that on mainthread the owner global is an inner window, and I'm pretty sure that the code I'm touching is all mainthread; I should have just dropped the CheckInnerWindowCorrectness bit there), and imposing a tax on them in the form of unused out params seems annoying.  Or put another way, XMLHttpRequest is the one place that's not DOMEventTargetHelper::WantsUntrusted where we care about the non-window-global case.

I'm not sure how to best make this work, honestly.  I guess I could add the outparam back; it just seems weird to have code that passes an nsresult* in and then completely ignores the nsresult, which is what we used to have.

> Though, I wonder whether other browsers use GetDocumentURI() or GetOriginalURI()

Other places where we send referrers we use GetDocumentURI(), so I'm just making this consistent with the other places.

Spec at https://fetch.spec.whatwg.org/#concept-main-fetch says to do https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer which says to "Let referrerSource be document’s URL." if we ignore the srcdoc and whatnot cases.  Clearly we need to define a GetReferrerURI or something on documents to actually implement that algorithm correctly...  I'll file a bug on that.

Anyway, do you feel strongly that we should have the ourparam nsresult thing?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1265961 on srcdoc referrers.

Comment 15

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

OK, I think I can live without the outparam.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b86bd948f296
https://hg.mozilla.org/integration/mozilla-inbound/rev/2299e6ad7403
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3965e363d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5eba3f4be30
https://hg.mozilla.org/integration/mozilla-inbound/rev/300b011666d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/77721ee6f814

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b86bd948f296
https://hg.mozilla.org/mozilla-central/rev/2299e6ad7403
https://hg.mozilla.org/mozilla-central/rev/2a3965e363d9
https://hg.mozilla.org/mozilla-central/rev/b5eba3f4be30
https://hg.mozilla.org/mozilla-central/rev/300b011666d1
https://hg.mozilla.org/mozilla-central/rev/77721ee6f814