Consider removing nsContentUtils::GetDocumentFromScriptContext()

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: bkelly, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(6 attachments, 1 obsolete attachment)

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.
Whiteboard: btpp-active
Attachment #8742584 - Attachment is obsolete: true
Attachment #8742584 - Flags: review?(bugs)
Blocks: 1265590
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.
Attachment #8742580 - Flags: review?(bugs) → review+
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 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
Attachment #8742586 - Flags: review?(bugs) → review+
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.
Attachment #8742581 - Flags: review?(bugs) → review-
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 :))
Attachment #8742585 - Flags: review?(bugs) → review+
> 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?
Flags: needinfo?(bugs)
OK, I think I can live without the outparam.
Flags: needinfo?(bugs)
Attachment #8742581 - Flags: review- → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.