Replace nsPIDOMWindow with nsPIDOMWindowInner/Outer

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
a month ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

(Depends on 1 bug)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

This bug will do steps 3 and 4 of bug 558192 comment 6.

We will move nsIDOMWindow to being a legacy XPIDL type like nsIDOMWindowInternal.  Two new XPIDL interfaces will be introduced, mozIDOMWindow and mozIDOMWindowProxy, corresponding to inner and outer windows respectively.  This will allow XPIDL entry points to specify whether they take inner or outer windows.

Then nsPIDOMWindow will be replaced by nsPIDOMWindowInner (inheriting from mozIDOMWindow) and nsPIDOMWindowOuter (inheriting from mozIDOMWindowProxy).  nsGlobalWindow will continue to be the concrete implementation class for all types.

When the dust settles every use of nsIDOMWindow and nsPIDOMWindow in our code today will use a type that specifies whether the window we have is an inner or outer window.
Posted patch Patch (obsolete) — Splinter Review
Attachment #8711825 - Flags: review?(mrbkap)
Comment on attachment 8711825 [details] [diff] [review]
Patch

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

Some over the shoulder comments from mrbkap.

::: docshell/base/nsDocShell.cpp
@@ +3566,5 @@
>  static bool
>  ItemIsActive(nsIDocShellTreeItem* aItem)
>  {
> +  if (nsCOMPtr<nsPIDOMWindowOuter> window = aItem->GetWindow()) {
> +    auto* win = static_cast<nsGlobalWindow*>(reinterpret_cast<nsPIDOMWindow<nsISupports>*>(window.get()));

nsGlobalWindow::Cast

::: dom/base/ScriptSettings.cpp
@@ +425,2 @@
>  {
> +  return Init(static_cast<nsGlobalWindow*>(reinterpret_cast<nsPIDOMWindow<nsISupports>*>(aWindow)), aCx);

nsGlobalWindow::Cast

@@ +431,2 @@
>  {
> +  return Init(static_cast<nsGlobalWindow*>(reinterpret_cast<nsPIDOMWindow<nsISupports>*>(aWindow)));

ibid

@@ +449,2 @@
>  {
> +  return InitWithLegacyErrorReporting(static_cast<nsGlobalWindow*>(reinterpret_cast<nsPIDOMWindow<nsISupports>*>(aWindow)));

...

::: dom/base/nsFocusManager.cpp
@@ +368,3 @@
>  {
>    // only top-level windows can be made active
> +  nsCOMPtr<nsPIDOMWindowOuter> piWindow = nsPIDOMWindowOuter::From(aWindow);

null check first

@@ +433,5 @@
>  {
>    // the focus method is stored on the inner window
> +  nsCOMPtr<nsPIDOMWindowOuter> window;
> +  if (aWindow)
> +    window = nsPIDOMWindowOuter::From(aWindow);

braces!

::: dom/base/nsGlobalWindow.cpp
@@ +1018,5 @@
>                                        JS::Handle<jsid> id,
>                                        JS::MutableHandle<JS::Value> vp,
>                                        bool& found) const
>  {
> +  nsCOMPtr<nsPIDOMWindowOuter>frame = GetSubframeWindow(cx, proxy, id);

' ' after >

@@ +2960,5 @@
>    // are GC-owned.
>    for (RefPtr<nsGlobalWindow> inner = (nsGlobalWindow *)PR_LIST_HEAD(this);
>         inner != this;
>         inner = (nsGlobalWindow*)PR_NEXT_LINK(inner)) {
> +    MOZ_ASSERT(!inner->mOuterWindow || inner->mOuterWindow == this->AsOuter());

skip this->

@@ +3024,1 @@
>  #endif

Remove this assertion.

@@ +3270,5 @@
>    nsContentUtils::GetLocalizedString(nsContentUtils::eCOMMON_DIALOG_PROPERTIES,
>                                       "ScriptDialogLabel", label);
>    nsContentUtils::GetLocalizedString(nsContentUtils::eCOMMON_DIALOG_PROPERTIES,
>                                       "ScriptDialogPreventTitle", title);
> +  promptSvc->Confirm(this->AsOuter(), title.get(), label.get(), &disableDialog);

Skip this->

@@ +3579,5 @@
>  {
>    MOZ_RELEASE_ASSERT(IsInnerWindow());
>  
>    if (!mNavigator) {
> +    mNavigator = new Navigator(this->AsInner());

Skip this->

@@ +3602,5 @@
>  {
>    MOZ_RELEASE_ASSERT(IsInnerWindow());
>  
>    if (!mScreen) {
> +    mScreen = nsScreen::Create(this->AsInner());

ibid

@@ +3629,5 @@
>  {
>    MOZ_RELEASE_ASSERT(IsInnerWindow());
>  
>    if (!mHistory) {
> +    mHistory = new nsHistory(this->AsInner());

ibid

@@ +3780,5 @@
>  {
>    MOZ_RELEASE_ASSERT(IsInnerWindow());
>  
>    if (!mSpeechSynthesis) {
> +    mSpeechSynthesis = new SpeechSynthesis(this->AsInner());

...

@@ +3814,2 @@
>    if (mDocShell->GetIsBrowserOrApp()) {
> +    parent = this->AsOuter();

...

@@ +3863,4 @@
>      return win.forget();
>    }
>  
> +  nsCOMPtr<nsPIDOMWindowOuter> win(this->AsOuter());

....

@@ +4343,5 @@
>      nsContentUtils::GetOfflineAppManifest(mDoc, getter_AddRefs(manifestURI));
>  
>      RefPtr<nsDOMOfflineResourceList> applicationCache =
> +      new nsDOMOfflineResourceList(manifestURI, uri, mDoc->NodePrincipal(),
> +                                   this->AsInner());

...

@@ +4539,5 @@
>        aError.Throw(NS_ERROR_DOM_SECURITY_ERR);
>        return;
>      }
>  
> +    win = xpc::WindowOrNull(unwrapped)->AsInner();

null check?
add a test?

@@ +6486,5 @@
>      return false;
>    }
>  
>    nsCOMPtr<nsIPrompt> prompt;
> +  aError = promptFac->GetPrompt(this->AsOuter(), NS_GET_IID(nsIPrompt),

this!

@@ +6605,5 @@
>      return;
>    }
>  
>    nsCOMPtr<nsIPrompt> prompt;
> +  aError = promptFac->GetPrompt(this->AsOuter(), NS_GET_IID(nsIPrompt),

...

@@ +6720,5 @@
>    // about:blank loaded.  We don't want to focus our widget in that case.
>    // XXXbz should we really be checking for IsInitialDocument() instead?
>    bool lookForPresShell = true;
>    if (mDocShell->ItemType() == nsIDocShellTreeItem::typeChrome &&
> +      GetPrivateRoot() == this->AsOuter() && mDoc) {

...

@@ +6758,5 @@
>    if (canFocus) {
>      // if there is no parent, this must be a toplevel window, so raise the
>      // window if canFocus is true. If this is a child process, the raise
>      // window request will get forwarded to the parent by the puppet widget.
> +    aError = fm->SetActiveWindow(this->AsOuter());

...

@@ +6803,5 @@
>      // if the root is focused, clear the focus
>      nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>      if (fm && mDoc) {
>        nsCOMPtr<nsIDOMElement> element;
> +      fm->GetFocusedElementForWindow(this->AsOuter(), false, nullptr, getter_AddRefs(element));

...

@@ +6808,3 @@
>        nsCOMPtr<nsIContent> content = do_QueryInterface(element);
>        if (content == mDoc->GetRootElement())
> +        fm->ClearFocus(this->AsOuter());

...

@@ +7598,5 @@
>    if (!IsPopupBlocked(mDoc))
>      return true;
>  
> +  nsCOMPtr<nsPIDOMWindowOuter> parent = GetParent();
> +  if (parent == this->AsOuter())

...

@@ +7829,3 @@
>  nsGlobalWindow::GetFramesOuter()
>  {
> +  RefPtr<nsPIDOMWindowOuter> frames(this->AsOuter());

...

@@ +8298,5 @@
>             from the list of browsers) (and has an unload handler
>             that closes the window). */
>          // XXXbz now that we have mHavePendingClose, is this needed?
>          bool isTab = false;
> +        if (rootWin == this->AsOuter() ||

...

@@ +8553,3 @@
>      if (!skipNukeCrossCompartment && window) {
> +      nsCOMPtr<mozIDOMWindow> inner = do_QueryInterface(window);
> +      nsCOMPtr<mozIDOMWindowProxy> outer = do_QueryInterface(window);

Upcast to nsGlobalWindow and use that directly.

@@ +9093,5 @@
>    // should be the current search frame.
>    nsCOMPtr<nsIWebBrowserFindInFrames> framesFinder(do_QueryInterface(finder));
>    if (framesFinder) {
> +    framesFinder->SetRootSearchFrame(this->AsOuter());   // paranoia
> +    framesFinder->SetCurrentSearchFrame(this->AsOuter());

...

@@ +9167,5 @@
> +nsPIDOMWindowOuter*
> +nsGlobalWindow::GetOwnerGlobalForBindings()
> +{
> +  if (IsOuterWindow()) {
> +    return this->AsOuter();

Remove this->

@@ +9343,2 @@
>  
> +  if (this->AsOuter() == parent) {

...

@@ +9390,5 @@
>    MOZ_RELEASE_ASSERT(IsInnerWindow());
>  
>    nsIDocShell *docShell = GetDocShell();
>    if (!mLocation && docShell) {
> +    mLocation = new nsLocation(this->AsInner(), docShell);

...

@@ +9544,5 @@
>    // update the chrome event handler on all our inner windows
>    for (nsGlobalWindow *inner = (nsGlobalWindow *)PR_LIST_HEAD(this);
>         inner != this;
>         inner = (nsGlobalWindow*)PR_NEXT_LINK(inner)) {
> +    NS_ASSERTION(!inner->mOuterWindow || inner->mOuterWindow == this->AsOuter(),

...

@@ +9724,5 @@
>    }
>  
>    nsIFocusManager* fm = nsFocusManager::GetFocusManager();
> +  if (fm) {
> +    fm->WindowShown(this->GetOuterWindow(), oldNeedsFocus);

...

@@ +9739,5 @@
>    // is being destroyed.
>  
>    nsIFocusManager* fm = nsFocusManager::GetFocusManager();
> +  if (fm) {
> +    fm->WindowHidden(this->GetOuterWindow());

...

@@ +9871,2 @@
>    AutoJSAPI jsapi;
> +  result = jsapi.Init(this->AsInner());

...

@@ +10015,5 @@
>  
>    if (!presShell) {
>      // Try flushing frames on our parent in case there's a pending
>      // style change that will create the presshell.
> +    nsGlobalWindow *parent = nsGlobalWindow::Cast(GetPrivateParent());

auto?

@@ +10104,5 @@
>        return nullptr;
>      }
>  
>      nsCOMPtr<nsIDOMStorage> storage;
> +    aError = storageManager->CreateStorage(this->AsInner(), principal, documentURI,

..

@@ +10141,5 @@
>      return nullptr;
>    }
>  
>    if (!mLocalStorage) {
> +    if (!DOMStorage::CanUseStorage(this->AsInner())) {

...

@@ +10165,5 @@
>        mDoc->GetDocumentURI(documentURI);
>      }
>  
>      nsCOMPtr<nsIDOMStorage> storage;
> +    aError = storageManager->CreateStorage(this->AsInner(), principal, documentURI,

...

@@ +10186,5 @@
>    MOZ_RELEASE_ASSERT(IsInnerWindow());
>    if (!mIndexedDB) {
>      // This may keep mIndexedDB null without setting an error.
> +    aError = IDBFactory::CreateForWindow(this->AsInner(),
> +                                         getter_AddRefs(mIndexedDB));

...

@@ +10292,3 @@
>  
>      nsContentUtils::StorageAccess access =
> +      nsContentUtils::StorageAllowedForWindow(this->AsInner());

...

@@ +11254,5 @@
>        docShell->GetChildAt(i, getter_AddRefs(childShell));
>        NS_ASSERTION(childShell, "null child shell");
>  
> +      if (nsCOMPtr<nsPIDOMWindowOuter> pWin = childShell->GetWindow()) {
> +        nsGlobalWindow *win = nsGlobalWindow::Cast(pWin);

auto?

@@ +11284,2 @@
>  
> +  if (parent && parent != this->AsOuter()) {

...

@@ +11433,5 @@
>  
>      if (!aCalledNoScript) {
>        // We asserted at the top of this function that aNavigate is true for
>        // !aCalledNoScript.
> +      rv = pwwatch->OpenWindow2(this->AsOuter(), url.get(), name_ptr,

...

@@ +11453,5 @@
>          nojsapi.emplace();
>        }
>  
>  
> +      rv = pwwatch->OpenWindow2(this->AsOuter(), url.get(), name_ptr,

...

@@ +11534,1 @@
>      if (!forwardTo) {

Followup on why this is here?

@@ +11538,5 @@
>  
>      // If the caller and the callee share the same outer window, forward to the
>      // caller inner. Else, we forward to the current inner (e.g. someone is
>      // calling setTimeout() on a reference to some other window).
> +    if (forwardTo->GetOuterWindow() != this->AsOuter() ||

...

@@ +11553,5 @@
>  
>    // If forwardTo is not the window with an active document then we want the
>    // call to setTimeout/Interval to be a noop, so return null but don't set an
>    // error.
> +  MOZ_ASSERT(forwardTo->IsInnerWindow());

Drop the redundant assertion.

@@ +12578,2 @@
>      } else {
> +      mozilla::dom::workers::SuspendWorkersForWindow(this->AsInner());

...

@@ +12618,5 @@
>        docShell->GetChildAt(i, getter_AddRefs(childShell));
>        NS_ASSERTION(childShell, "null child shell");
>  
> +      if (nsCOMPtr<nsPIDOMWindowOuter> pWin = childShell->GetWindow()) {
> +        nsGlobalWindow* win = nsGlobalWindow::Cast(pWin);

auto?

@@ +12670,2 @@
>      } else {
> +      mozilla::dom::workers::ResumeWorkersForWindow(this->AsInner());

...

@@ +12847,5 @@
>  
>  void
>  nsGlobalWindow::EnableTimeChangeNotifications()
>  {
> +  mozilla::time::AddWindowListener(this->AsInner());

...

@@ +12853,5 @@
>  
>  void
>  nsGlobalWindow::DisableTimeChangeNotifications()
>  {
> +  mozilla::time::RemoveWindowListener(this->AsInner());

...

@@ +12874,5 @@
>        // Multiple global windows can share a document. So only measure the
>        // document if it (a) doesn't have a global window, or (b) it's the
>        // primary document for the window.
> +      if (!mDoc->GetInnerWindow() ||
> +          mDoc->GetInnerWindow() == this->AsInner()) {

...

@@ +13672,5 @@
>  {
>    MOZ_RELEASE_ASSERT(IsInnerWindow());
>  
>    if (!mConsole) {
> +    mConsole = new Console(this->AsInner());

...

::: dom/base/nsGlobalWindow.h
@@ +537,5 @@
>  
>    inline nsGlobalWindow* GetScriptableTopInternal()
>    {
> +    nsPIDOMWindowOuter* top = GetScriptableTop();
> +    return static_cast<nsGlobalWindow*>((nsPIDOMWindow<nsISupports>*)top);

nsGlobalWindow::Cast?

::: dom/base/nsINode.cpp
@@ +1329,4 @@
>  nsINode::GetOwnerGlobalForBindings()
>  {
>    bool dummy;
> +  auto* window = static_cast<nsGlobalWindow*>(OwnerDoc()->GetScriptHandlingObject(dummy));

nsGlobalWindow::Cast

::: dom/base/nsPIDOMWindow.h
@@ +67,5 @@
>    UIStateChangeType_Set,
>    UIStateChangeType_Clear
>  };
>  
> +enum FullscreenReason

enum class?

@@ +771,5 @@
> +    return static_cast<nsPIDOMWindowInner*>(aFrom);
> +  }
> +
> +  // Returns true if this object has an outer window and it is the current inner
> +  // window of that outer. Only call this on inner windows.

Remove the obvious.

::: dom/base/nsPIDOMWindowInlines.h
@@ +1,1 @@
> +template<class T>

License header.

@@ +43,5 @@
> +template <class T>
> +bool
> +nsPIDOMWindow<T>::IsLoading() const
> +{
> +  const nsPIDOMWindowInner* win;

move inside the branch

@@ +71,5 @@
> +bool
> +nsPIDOMWindow<T>::IsHandlingResizeEvent() const
> +{
> +  if (IsOuterWindow()) {
> +  const nsPIDOMWindowInner* win = AsOuter()->GetCurrentInnerWindow();

indentation?

@@ +127,5 @@
> +{
> +  if (IsOuterWindow()) {
> +    return mInnerWindow->GetFocusedNode();
> +  }
> +  return mFocusedNode;

\n?

::: dom/interfaces/smil/nsIDOMTimeEvent.idl
@@ +5,5 @@
>  
>  #include "nsIDOMEvent.idl"
>  
> +interface mozIDOMWindowProxy;
> +

axe

::: js/xpconnect/src/XPCConvert.cpp
@@ +931,5 @@
> +            // want the outer!  nsGlobalWindow will give us the outer via
> +            // getInterface.
> +            if (iid->Equals(NS_GET_IID(mozIDOMWindowProxy))) {
> +                nsCOMPtr<nsIInterfaceRequestor> ireq = do_QueryInterface(iface);
> +                return ireq && NS_SUCCEEDED(ireq->GetInterface(*iid, dest));

Do this by getting the inner explicitly.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1177,2 @@
>        nsContentUtils::XPConnect()->GetNativeOfWrapper(cx, target));
> +  return static_cast<nsGlobalWindow*>(reinterpret_cast<nsPIDOMWindow<nsISupports>*>(piWin.get()));

nsGlobalWindow::Cast

@@ +1929,5 @@
>          nsAutoJSString name;
>          if (!name.init(cx, JSID_TO_STRING(id)))
>              return false;
> +        if (nsCOMPtr<nsPIDOMWindowOuter> childDOMWin = win->GetChildWindow(name)) {
> +            nsGlobalWindow* cwin = static_cast<nsGlobalWindow*>(reinterpret_cast<nsPIDOMWindow<nsISupports>*>(childDOMWin.get()));

And here.
Comment on attachment 8711825 [details] [diff] [review]
Patch

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

I did a few spot checks and things generally look good. I'd be good to get Olli's stamp on the event stuff. I'm more confident in this because the type system is helping us out. There are a couple of places where you left comments that should be addressed before landing. I'd also appreciate a comment in nsGlobalWindow explaining the nsPIDOMWindow<nsISupports> stuff.

::: dom/base/nsXMLHttpRequest.cpp
@@ +306,5 @@
>                         nsILoadGroup* aLoadGroup)
>  {
>    NS_ENSURE_ARG_POINTER(aPrincipal);
> +  /*
> +  if (nsCOMPtr<nsPIDOMWindowInner> win = do_QueryInterface(aGlobalObject)) {

This looks like it should just be removed.

::: dom/html/HTMLMediaElement.cpp
@@ +5032,5 @@
>  
>    if (aCapture != mAudioCapturedByWindow) {
>      if (aCapture) {
>        mAudioCapturedByWindow = true;
> +      nsCOMPtr<nsPIDOMWindowInner> window =OwnerDoc()->GetInnerWindow();

Nit: Space after =.

::: hal/sandbox/SandboxHal.cpp
@@ +521,5 @@
>                PBrowserParent *browserParent) override
>    {
>      // We give all content vibration permission.
> +    //    TabParent *tabParent = TabParent::GetFrom(browserParent);
> +    /* xxxkhuey wtf

Can't you just pass the nsPIDOMWindowInner through here?
Attachment #8711825 - Flags: review?(mrbkap) → review+

Updated

3 years ago
Blocks: 1243760
Posted patch PatchSplinter Review
Olli, could you look at the event handling changes?
Attachment #8711825 - Attachment is obsolete: true
Attachment #8713303 - Flags: review?(bugs)
Comment on attachment 8713303 [details] [diff] [review]
Patch

OT, I wonder why InitMessageEvent takes cx as param.

 MouseScrollEvent::Axis()
 {
   return mEvent->AsMouseScrollEvent()->isHorizontal ?
-           static_cast<int32_t>(HORIZONTAL_AXIS) :
-           static_cast<int32_t>(VERTICAL_AXIS);
+          static_cast<int32_t>(nsIDOMMouseScrollEvent::HORIZONTAL_AXIS) :
+          static_cast<int32_t>(nsIDOMMouseScrollEvent::VERTICAL_AXIS);
 }
 Oh, right, we don't expose webidl constants in any sane way to C++ :/


+void
+UIEvent::InitUIEvent(const nsAString& typeArg,
+                     bool canBubbleArg,
+                     bool cancelableArg,
+                     nsGlobalWindow* viewArg,
+                     int32_t detailArg)
I could complain about wrong naming of the params, but since the other init has the
same stuff, fine. (but if you fix this kind of stuff, fix this too)


So UIEvent has stored usually inner window currently, but your change makes it store outer.
I'd prefer inner, since C++ callers should usually play with inner window. But - I guess it doesn't matter much since there aren't C++ callers and the new setup guarantees that one knows which object is returned.
However, in general C++ side should play with inner whenever possible. History has shown that using outer leads easily to errors.

void InitCommandEvent(const nsAString& aType,
                         bool aCanBubble, bool aCancelable,
-                        nsIDOMWindow* aView,
+                        nsGlobalWindow* aView,
                         int32_t aDetail,
                         bool aCtrlKey, bool aAltKey,
                         bool aShiftKey, bool aMetaKey,
-                        Event* aSourceEvent,
-                        ErrorResult& aRv)
+                        Event* aSourceEvent)
   {
-    aRv = InitCommandEvent(aType, aCanBubble, aCancelable, aView, aDetail,
-                           aCtrlKey, aAltKey, aShiftKey, aMetaKey,
-                           aSourceEvent);
+    InitCommandEvent(aType, aCanBubble, aCancelable, aView->AsInner(),
+                     aDetail, aCtrlKey, aAltKey, aShiftKey, aMetaKey,
+                     aSourceEvent);
This is a tad error prone that aView->AsInner() is called always without knowing that aView is inner, but
I guess C++ callers should be caught asap by the assertion in AsInner(). so fine.




Not about events but..
 AudioTrackList*
 HTMLMediaElement::AudioTracks()
 {
   if (!mAudioTrackList) {
-    nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(OwnerDoc()->GetParentObject());
+    nsCOMPtr<nsPIDOMWindowInner> window = OwnerDoc()->GetInnerWindow();
     mAudioTrackList = new AudioTrackList(window, this);
   }
   return mAudioTrackList;
 }

Why is that ok? GetParentObject() may return non-null in cases when GetInnerWindow() returns null.
data document do have parent object, but don't have inner window.
Same also elsewhere.


   // This is safe since we're on main-thread, and the windowlist can only
   // be added to from the main-thread
-  nsPIDOMWindow *window = static_cast<nsPIDOMWindow*>
-      (nsGlobalWindow::GetInnerWindowWithId(aWindowID));
+  nsPIDOMWindowInner* window =
+    nsGlobalWindow::GetInnerWindowWithId(aWindowID)->AsInner();
   if (window) {
Why it is safe to call ->AsInner()? GetInnerWindowWithId may return null, afais.
Same also elsewhere.




anyhow, r+ for the event stuff, although, I'd prefer storing inner window as member variables, not outer.
But I can live with the setup the patch has.
(looks like also TimeEvent has mView similar to UIEvent)
Attachment #8713303 - Flags: review?(bugs) → review+

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e22b3043887e
https://hg.mozilla.org/mozilla-central/rev/998d61de14ee
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Comment 9

3 years ago
Looks like there's an issue in toolkit/:

In file included from /Users/buildbot/buildslave/macosx/build/mozilla/toolkit/components/downloads/nsDownloadManager.cpp:25:
In file included from /Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsILocalFileWin.h:10:
In file included from /Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsILocalFile.h:10:
In file included from /Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsIFile.h:920:
In file included from /Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsDirectoryServiceUtils.h:10:
In file included from /Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsIServiceManager.h:138:
In file included from /Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsComponentManagerUtils.h:11:
/Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsCOMPtr.h:390:5: error: static_assert failed "nsCOMPtr only works for types with IIDs.  Either use RefPtr; add an IID to your type with NS_DECLARE_STATIC_IID_ACCESSOR/NS_DEFINE_STATIC_IID_ACCESSOR; or make the nsCOMPtr point to a base class with an IID."
    static_assert(1 < sizeof(TestForIID<T>(nullptr)), "nsCOMPtr only works "
    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsCOMPtr.h:430:5: note: in instantiation of member function 'nsCOMPtr<mozIDOMWindowProxy>::assert_validity' requested here
    assert_validity();
    ^
/Users/buildbot/buildslave/macosx/build/mozilla/toolkit/components/downloads/nsDownloadManager.cpp:2576:32: note: in instantiation of member function 'nsCOMPtr<mozIDOMWindowProxy>::nsCOMPtr' requested here
  nsCOMPtr<mozIDOMWindowProxy> dmWindow;
                               ^
1 error generated.
Sounds like some sort of unified build issue.  An include missing somewhere, most likely.

Comment 11

3 years ago
Adding #include "mozIDOMWindow.h" to nsDownloadManager.h fixes the build failure, but I'm not sure if it's really the right place to add it.

Comment 12

3 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> Sounds like some sort of unified build issue.  An include missing somewhere,
> most likely.

It's not a OSX unified build issue as it also happens on Linux.
(In reply to aleth [:aleth] from comment #12)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> > Sounds like some sort of unified build issue.  An include missing somewhere,
> > most likely.
> 
> It's not a OSX unified build issue as it also happens on Linux.

I believe Kyle is referring to the "unified compilation" technique where multiple C++ files are compiled together as a group to reduce compile time.  This is done on all platforms and can cause compilation errors when disabled since there can be #includes missing in the individual files that aren't needed when compiled as group.
Depends on: 1244582
Depends on: 1244726

Updated

3 years ago
Depends on: 1249870

Updated

3 years ago
Depends on: 1251207
Depends on: 1257444
Depends on: 1262930
Depends on: 1369246
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.