861729 - Remove GetExtantDocument

Status: RESOLVED FIXED

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

Description

[David Zbarsky (:dzbarsky)]

Attachment: Patch

The majority of callers QI to nsIDocument instead of using GetExtantDoc()

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 737344 [details] [diff] [review]
Patch

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

I really like this, but you're making a lot of strong references weak, and that scares me a lot ever since I introduced some use-after-frees that way. I'll try to review more carefully later.

::: content/events/src/nsEventListenerManager.cpp
@@ +327,5 @@
>               aTypeAtom == nsGkAtoms::onmouseleave) {
>      mMayHaveMouseEnterLeaveEventListener = true;
>      nsPIDOMWindow* window = GetInnerWindowForTarget();
>      if (window) {
>  #ifdef DEBUG

You can drop the ifdef now

::: dom/base/nsDOMWindowUtils.cpp
@@ +2936,5 @@
>  
>    nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
>    NS_ENSURE_STATE(window);
>  
> +  nsIDocument* doc = window->GetExtantDoc();

We used to throw if the document was null, now we crash.

::: dom/indexedDB/IDBDatabase.h
@@ +97,3 @@
>    }
>  
>    already_AddRefed<nsIDocument> GetOwnerDocument()

Followup to make this return a raw pointer

::: dom/power/WakeLock.cpp
@@ +172,4 @@
>    if (window) {
> +    nsIDocument* doc = window->GetExtantDoc();
> +    if (doc) {
> +      nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(doc);

No need for the QI

@@ +198,5 @@
>  
>    if (window) {
> +    nsIDocument* doc = window->GetExtantDoc();
> +    if (doc) {
> +      nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(doc);

Ditto

Comment 2

[Masatoshi Kimura [:emk]]

Comment on attachment 737344 [details] [diff] [review]
Patch

Could you also remove nsPIDOMWindow::mDocument?

Comment 3

[David Zbarsky (:dzbarsky)]

(In reply to Masatoshi Kimura [:emk] from comment #2)
> Comment on attachment 737344 [details] [diff] [review]
> Patch
> 
> Could you also remove nsPIDOMWindow::mDocument?
'
Yup, I was going to do that in a followup.

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Remove nsPIDOMWindow::mDocument

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 737344 [details] [diff] [review]
Patch

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

I've thought about this and I'm not confident I can review the strong -> raw pointer changes. I would be happy to review if you reintroduced the strong pointers you removed.

Comment 6

[David Zbarsky (:dzbarsky)]

Attachment: Patch

I switched back to strong pointers everywhere.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 738276 [details] [diff] [review]
Remove nsPIDOMWindow::mDocument

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

::: dom/base/nsGlobalWindow.cpp
@@ +6166,5 @@
>    }
>  
>    bool blocked = true;
> +
> +  if (aDoc) {

if (!aDoc) {
  return true;
}
...
return permission == nsIPopupWindowManager::DENY_POPUP;

@@ +6781,2 @@
>    if (!event)
> +    return res.ErrorCode();

This used to not throw, please keep that.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 739225 [details] [diff] [review]
Patch

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

Thanks!

::: dom/base/Navigator.cpp
@@ +625,5 @@
>  
>  class VibrateWindowListener : public nsIDOMEventListener
>  {
>  public:
> +  VibrateWindowListener(nsIDOMWindow *aWindow, nsIDocument *aDocument)

* to the left

::: dom/base/nsGlobalWindow.cpp
@@ +6360,5 @@
>                  getter_AddRefs(popupURI));
>  
>    // fire an event chock full of informative URIs
> +  if (aBlocked) {
> +    nsCOMPtr<nsIDocument> topdoc = do_QueryInterface(topDoc);

Variables whose names only differ in case... :/

Maybe topDocument?

::: dom/src/jsurl/nsJSProtocolHandler.cpp
@@ +652,5 @@
>          }
>      }
>  
>      mDocumentOnloadBlockedOn =
> +        mOriginalInnerWindow->GetExtantDoc();

I think this fits on one line

::: embedding/components/windowwatcher/src/nsAutoWindowStateHelper.cpp
@@ +12,4 @@
>  #include "nsIDOMEventTarget.h"
>  #include "nsIDOMEvent.h"
>  #include "nsString.h"
>  #include "nsGUIEvent.h"

Just sort these, please. (But keep nsAutoWindowStateHelper.h on its own.)

Comment 9

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2201d650ac90
https://hg.mozilla.org/integration/mozilla-inbound/rev/f710f8fb2ab0

Comment 10

[David Zbarsky (:dzbarsky)]

Backed out for not building on Windows 
https://hg.mozilla.org/integration/mozilla-inbound/rev/eefc8051c72f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b7ed89e98a3

Comment 11

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95262d693642
https://hg.mozilla.org/integration/mozilla-inbound/rev/787e5340d7af

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95262d693642
https://hg.mozilla.org/mozilla-central/rev/787e5340d7af