Closed
Bug 1265072
Opened 8 years ago
Closed 8 years ago
Consider removing nsContentUtils::GetDocumentFromScriptContext()
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: bkelly, Assigned: bzbarsky)
References
Details
(Whiteboard: btpp-active)
Attachments
(6 files, 1 obsolete file)
3.40 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
2.95 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
1.29 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
3.57 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
7.16 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
2.55 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•8 years ago
|
Whiteboard: btpp-active
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8742580 -
Flags: review?(bugs)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8742581 -
Flags: review?(bugs)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8742582 -
Flags: review?(bugs)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8742583 -
Flags: review?(bugs)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8742584 -
Flags: review?(bugs)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8742585 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8742584 -
Attachment is obsolete: true
Attachment #8742584 -
Flags: review?(bugs)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8742586 -
Flags: review?(bugs)
Comment 8•8 years ago
|
||
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 9•8 years ago
|
||
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•8 years ago
|
||
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
Updated•8 years ago
|
Attachment #8742586 -
Flags: review?(bugs) → review+
Comment 11•8 years ago
|
||
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-
Updated•8 years ago
|
Attachment #8742582 -
Flags: review?(bugs)
Updated•8 years ago
|
Attachment #8742583 -
Flags: review?(bugs)
Comment 12•8 years ago
|
||
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+
Assignee | ||
Comment 13•8 years ago
|
||
> 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)
Assignee | ||
Comment 14•8 years ago
|
||
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1265961 on srcdoc referrers.
Updated•8 years ago
|
Attachment #8742581 -
Flags: review- → review+
Updated•8 years ago
|
Attachment #8742582 -
Flags: review+
Updated•8 years ago
|
Attachment #8742583 -
Flags: review+
Comment 16•8 years ago
|
||
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•8 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•