Closed Bug 894646 Opened 7 years ago Closed 7 years ago

Various dom deCOM cleanups

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Details

Attachments

(4 files)

Attached patch PatchSplinter Review
No description provided.
Attachment #776725 - Attachment is patch: true
Attachment #776725 - Attachment mime type: message/rfc822 → text/plain
Attachment #776725 - Flags: review?(Ms2ger)
Comment on attachment 776725 [details] [diff] [review]
Patch

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

A few bugs and crashes... Please attach an interdiff when you address the comments.

::: content/base/src/nsDocument.cpp
@@ +6620,5 @@
>    switch (adoptedNode->NodeType()) {
>      case nsIDOMNode::ATTRIBUTE_NODE:
>      {
>        // Remove from ownerElement.
>        nsCOMPtr<nsIDOMAttr> adoptedAttr = do_QueryInterface(adoptedNode);

Let's just cast here, and make this an nsRefPtr<Attr>

@@ +6623,5 @@
>        // Remove from ownerElement.
>        nsCOMPtr<nsIDOMAttr> adoptedAttr = do_QueryInterface(adoptedNode);
>        NS_ASSERTION(adoptedAttr, "Attribute not implementing nsIDOMAttr");
>  
> +      ErrorResult rv;

You're shadowing the rv argument here.

::: content/xul/document/src/nsXULCommandDispatcher.cpp
@@ +209,2 @@
>    if (frameElement)
>      return fm->SetFocus(frameElement, 0);

Followup to fix SetFocus?

::: dom/base/nsContentPermissionHelper.cpp
@@ +92,5 @@
>    if (mParent == nullptr) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  CallQueryInterface(mParent->mElement, aRequestingElement);

This is crashy if mParent->mElement is null.

::: dom/base/nsContentPermissionHelper.h
@@ +27,4 @@
>    virtual ~ContentPermissionRequestParent();
>  
>    nsCOMPtr<nsIPrincipal> mPrincipal;
> +  nsCOMPtr<Element>      mElement;

One space is sufficient

::: dom/base/nsDOMWindowUtils.cpp
@@ +1289,5 @@
>    NS_ENSURE_STATE(doc);
>  
>    Element* el =
>      doc->ElementFromPointHelper(aX, aY, aIgnoreRootScrollFrame, aFlushLayout);
> +  CallQueryInterface(el, aReturn);

Crash

::: dom/base/nsGlobalWindow.cpp
@@ +3097,5 @@
>      // This is page load event since load events don't propagate to |window|.
>      // @see nsDocument::PreHandleEvent.
>      mIsDocumentLoaded = true;
>  
> +    nsCOMPtr<nsIContent> content = GetFrameElementInternal();

Element

@@ +6370,5 @@
>    // (see nsWindowWatcher::URIfromURL)
>  
>    // first, fetch the opener's base URI
>  
>    nsIURI *baseURL = 0;

nullptr while you're here

@@ +7606,5 @@
>      // element to content code.
>      return NS_OK;
>    }
>  
> +  CallQueryInterface(mFrameElement, aFrameElement);

Crash

@@ +10858,5 @@
>        sourceWindow = do_QueryInterface(scriptcx->GetGlobalObject());
>    }
>  
>    if (!sourceWindow) {
> +    sourceWindow = do_QueryInterface(NS_ISUPPORTS_CAST(nsPIDOMWindow *, this));

Get rid of this nonsense, please.

@@ +11057,5 @@
>          nsGlobalWindow* inner = win->GetCurrentInnerWindowInternal();
>  
>          // This is a bit hackish. Only freeze/suspend windows which are truly our
>          // subwindows.
> +        nsCOMPtr<nsIContent> frame = pWin->GetFrameElementInternal();

Element

@@ +11169,5 @@
>          nsGlobalWindow* inner = win->GetCurrentInnerWindowInternal();
>  
>          // This is a bit hackish. Only thaw/resume windows which are truly our
>          // subwindows.
> +        nsCOMPtr<nsIContent> frame = pWin->GetFrameElementInternal();

Ditto

::: dom/base/nsLocation.cpp
@@ +42,5 @@
>  GetDocumentCharacterSetForURI(const nsAString& aHref, nsACString& aCharset)
>  {
>    aCharset.Truncate();
>  
>    nsresult rv;

Unused variable

@@ +51,4 @@
>        do_QueryInterface(nsJSUtils::GetDynamicScriptGlobal(cx));
>      NS_ENSURE_TRUE(window, NS_ERROR_FAILURE);
>  
> +    if (nsIDocument* doc = window->GetDoc()) {

Double-check if you need to hold a ref here

::: dom/base/nsPIDOMWindow.h
@@ +196,5 @@
>    // Internal getter/setter for the frame element, this version of the
>    // getter crosses chrome boundaries whereas the public scriptable
>    // one doesn't for security reasons.
> +  mozilla::dom::Element* GetFrameElementInternal() const;
> +  void SetFrameElementInternal(mozilla::dom::Element* aFrameElement);

Ask someone if those are perf-sensitive

::: dom/ipc/ContentParent.cpp
@@ +399,1 @@
>      if (appType != NS_LITERAL_STRING("critical")) {

AttrValueIs

::: dom/ipc/TabChild.cpp
@@ +543,5 @@
>    }
>  
>    float minScale = 1.0f;
>  
> +  Element* htmlDOMElement = document->GetHtmlElement();

Let's keep this a strong pointer

::: dom/ipc/TabParent.cpp
@@ +316,5 @@
>      nsCOMPtr<nsIDOMElement> dummy;
>      uint32_t type = aForward ? uint32_t(nsIFocusManager::MOVEFOCUS_FORWARD)
>                               : uint32_t(nsIFocusManager::MOVEFOCUS_BACKWARD);
> +    nsCOMPtr<nsIDOMElement> frame = do_QueryInterface(mFrameElement);
> +    fm->MoveFocus(nullptr, frame, type, nsIFocusManager::FLAG_BYKEY,

Followup

@@ +1242,5 @@
>  TabParent::HandleDelayedDialogs()
>  {
>    nsCOMPtr<nsIWindowWatcher> ww = do_GetService(NS_WINDOWWATCHER_CONTRACTID);
>    nsCOMPtr<nsIDOMWindow> window;
> +  if (mFrameElement) {

Can mFrameElement change from under us?

@@ +1259,1 @@
>        dialogCreator->OpenDialog(data->mType,

Followup

::: dom/ipc/TabParent.h
@@ +62,5 @@
>  
>  public:
>      TabParent(ContentParent* aManager, const TabContext& aContext);
>      virtual ~TabParent();
> +    Element* GetOwnerElement() { return mFrameElement; }

const

::: dom/system/nsDeviceSensors.cpp
@@ +218,4 @@
>        if (type == nsIDeviceSensorData::TYPE_ACCELERATION ||
>          type == nsIDeviceSensorData::TYPE_LINEAR_ACCELERATION ||
>          type == nsIDeviceSensorData::TYPE_GYROSCOPE)
> +        FireDOMMotionEvent(doc, doc, type, x, y, z);

Uh, I don't think so

::: layout/inspector/src/inLayoutUtils.cpp
@@ +103,2 @@
>  {
> +  nsPIDOMWindow *pwin = aDoc.GetWindow();

* to the left

@@ +105,1 @@
>    if (!pwin) return nullptr;

{}
Attachment #776725 - Flags: review?(Ms2ger) → review-
(In reply to :Ms2ger from comment #1)
> Comment on attachment 776725 [details] [diff] [review]
> Patch
> 

> 
> Followup to fix SetFocus?
> 

nsIFocusManager is defined in idl.

> 
> @@ +51,4 @@
> >        do_QueryInterface(nsJSUtils::GetDynamicScriptGlobal(cx));
> >      NS_ENSURE_TRUE(window, NS_ERROR_FAILURE);
> >  
> > +    if (nsIDocument* doc = window->GetDoc()) {
> 
> Double-check if you need to hold a ref here
> 

This just returns a string.


> @@ +1242,5 @@
> >  TabParent::HandleDelayedDialogs()
> >  {
> >    nsCOMPtr<nsIWindowWatcher> ww = do_GetService(NS_WINDOWWATCHER_CONTRACTID);
> >    nsCOMPtr<nsIDOMWindow> window;
> > +  if (mFrameElement) {
> 
> Can mFrameElement change from under us?
> 

Nope.  TabParents are created for specific iframes, which are then set as the owner element.  mFrameElement is never changed after that.

> @@ +1259,1 @@
> >        dialogCreator->OpenDialog(data->mType,
> 
> Followup

nsIDialogCreator.idl

> 
> ::: dom/system/nsDeviceSensors.cpp
> @@ +218,4 @@
> >        if (type == nsIDeviceSensorData::TYPE_ACCELERATION ||
> >          type == nsIDeviceSensorData::TYPE_LINEAR_ACCELERATION ||
> >          type == nsIDeviceSensorData::TYPE_GYROSCOPE)
> > +        FireDOMMotionEvent(doc, doc, type, x, y, z);
> 
> Uh, I don't think so
> 

Thanks, good catch.
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #778194 - Flags: review?(Ms2ger)
Comment on attachment 778194 [details] [diff] [review]
Addressing review comments

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

::: content/base/src/nsDocument.cpp
@@ +6631,5 @@
>          if (rv.Failed()) {
>            return nullptr;
>          }
>  
>          newAttr.swap(adoptedAttr);

Does this compile?

::: dom/system/nsDeviceSensors.cpp
@@ +217,5 @@
>      if (doc) {
>        if (type == nsIDeviceSensorData::TYPE_ACCELERATION ||
>          type == nsIDeviceSensorData::TYPE_LINEAR_ACCELERATION ||
>          type == nsIDeviceSensorData::TYPE_GYROSCOPE)
> +        FireDOMMotionEvent(doc, pwindow, type, x, y, z);

Does this compile? pwindow is not an EventTarget...

@@ +223,5 @@
>          FireDOMOrientationEvent(doc, doc, x, y, z);
>        else if (type == nsIDeviceSensorData::TYPE_PROXIMITY)
>          FireDOMProximityEvent(doc, x, y, z);
>        else if (type == nsIDeviceSensorData::TYPE_LIGHT)
>          FireDOMLightEvent(doc, x);

You also need to revert these three.
Attachment #778194 - Flags: review?(Ms2ger) → review-
Sorry, I could have sworn I tried compiling before posting the last interdiff, but apparently not.
Attachment #779649 - Flags: review?(Ms2ger)
Attached patch Rebased patchSplinter Review
Comment on attachment 779657 [details] [diff] [review]
Rebased patch

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

r=me with the following addressed. Thanks!

::: content/base/src/nsFrameLoader.cpp
@@ +1647,5 @@
>    // mDocShell->SetChromeEventHandler() for the global window to get
>    // the right chrome event handler.
>  
>    // Tell the window about the frame that hosts it.
> +  nsCOMPtr<Element> frame_element = do_QueryInterface(mOwnerContent);

mOwnerContent is a dom::Element*. Drop the QI, but keep the ref.

@@ +2062,5 @@
>      // The |else| above is unnecessary; OwnerIsBrowserFrame() implies !ownApp.
>      context.SetTabContextForBrowserFrame(containingApp, scrollingBehavior);
>    }
>  
> +  nsCOMPtr<Element> ownerElement = do_QueryInterface(mOwnerContent);

Ditto

::: dom/base/nsContentPermissionHelper.cpp
@@ +8,5 @@
>  #include "nsIDOMWindow.h"
>  #include "nsIDOMElement.h"
>  #include "nsIPrincipal.h"
>  #include "mozilla/unused.h"
> +#include "mozilla/dom/Element.h"

mozilla/d before mozilla/u

::: dom/base/nsContentPermissionHelper.h
@@ +23,4 @@
>  class ContentPermissionRequestParent : public PContentPermissionRequestParent
>  {
>   public:
> +  ContentPermissionRequestParent(const nsACString& type, const nsACString& access, Element* element, const IPC::Principal& principal);

Nit: 80 columns

::: dom/base/nsDOMWindowUtils.cpp
@@ +1289,5 @@
>    NS_ENSURE_STATE(doc);
>  
> +  nsCOMPtr<nsIDOMElement> el = do_QueryInterface(
> +    doc->ElementFromPointHelper(aX, aY, aIgnoreRootScrollFrame, aFlushLayout));
> +  el.forget(aReturn);

Since you're not actually changing anything here, let's drop this change.

@@ +3222,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    return NS_OK;

This can be 'return doc->...'. Up to you.

::: dom/base/nsGlobalWindow.cpp
@@ +6290,5 @@
>      }
>    }
>  }
>  
> +void FirePopupWindowEvent(nsIDocument* aDoc)

Nit: add 'static', so we notice if it ever becomes unused.

@@ +6391,1 @@
>      return;

{}

@@ +6409,5 @@
>        contextWindow = do_QueryInterface(currentCX->GetGlobalObject());
>      }
>    }
>    if (!contextWindow)
> +    contextWindow = this;

{}

@@ +9160,5 @@
>    } else {
>      name.AssignLiteral("online");
>    }
>    // The event is fired at the body element, or if there is no body element,
>    // at the document.

(What a mess.)

@@ +9161,5 @@
>      name.AssignLiteral("online");
>    }
>    // The event is fired at the body element, or if there is no body element,
>    // at the document.
>    nsCOMPtr<nsISupports> eventTarget = mDoc.get();

Can you make this an nsCOMPtr<EventTarget>? And try to drop the .get().

::: layout/inspector/src/inLayoutUtils.cpp
@@ +95,2 @@
>  
> +  return pwin->GetFrameElementInternal()->AsDOMNode();

Is this necessarily non-null?
Attachment #779657 - Flags: review+
Attachment #779649 - Flags: review?(Ms2ger)
> @@ +9161,5 @@
> >      name.AssignLiteral("online");
> >    }
> >    // The event is fired at the body element, or if there is no body element,
> >    // at the document.
> >    nsCOMPtr<nsISupports> eventTarget = mDoc.get();
> 
> Can you make this an nsCOMPtr<EventTarget>? And try to drop the .get().
> 

I changed to EventTarget, but you need the .get().

> ::: layout/inspector/src/inLayoutUtils.cpp
> @@ +95,2 @@
> >  
> > +  return pwin->GetFrameElementInternal()->AsDOMNode();
> 
> Is this necessarily non-null?

No, I changed to QI.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9dbb5dbb96a
Backed out the changes to nsDeviceSensors for not building on Windows.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c68b8da6ec5a
https://hg.mozilla.org/mozilla-central/rev/d9dbb5dbb96a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.