Status

()

defect
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

unspecified
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Posted patch Patch (obsolete) — Splinter Review
The majority of callers QI to nsIDocument instead of using GetExtantDoc()
Attachment #737344 - Flags: review?(Ms2ger)
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
Attachment #737344 - Flags: feedback+
Comment on attachment 737344 [details] [diff] [review]
Patch

Could you also remove nsPIDOMWindow::mDocument?
(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.
Attachment #738276 - Flags: review?(Ms2ger)
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.
Attachment #737344 - Flags: review?(Ms2ger)
Posted patch PatchSplinter Review
I switched back to strong pointers everywhere.
Attachment #737344 - Attachment is obsolete: true
Attachment #739225 - Flags: review?(Ms2ger)
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.
Attachment #738276 - Flags: review?(Ms2ger) → review+
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.)
Attachment #739225 - Flags: review?(Ms2ger) → review+
https://hg.mozilla.org/mozilla-central/rev/95262d693642
https://hg.mozilla.org/mozilla-central/rev/787e5340d7af
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.