1444143 - Remove nsIFrameLoader

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Boris Zbarsky [:bzbarsky]]

We have various unused stuff here.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove unused stuff from nsIFrameLoader

MozReview-Commit-ID: 3FpiEo2pxjr

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove nsIFrameLoader::GetDocShell

MozReview-Commit-ID: DwnWt8jTokV

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove nsIFrameLoader::GetTabParent

MozReview-Commit-ID: 2AgXdhJiunG

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Remove nsIFrameLoader::GetDepthTooGreat

MozReview-Commit-ID: GIdz853oVd3

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Remove nsIFrameLoader::GetOwnerIsMozBrowserFrame

MozReview-Commit-ID: 6IfTdTvDZtm

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Remove nsIFrameLoader::Get/SetEventMode

MozReview-Commit-ID: Jvevh2puiLY

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Remove nsIFrameLoader::Destroy

MozReview-Commit-ID: GObbSTCrjad

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Remove nsIFrameLoader::LoadFrame

MozReview-Commit-ID: Ackil1mtVy0

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Remove nsIFrameLoader::LoadURI

MozReview-Commit-ID: fSRCzBiHUE

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Remove unnecessary QIs to Ci.nsIFrameLoader in JS

nsFrameLoader is on WebIDL bindings, so those QIs are no-ops anyway, unless the given object is no a frameloader to start with.

MozReview-Commit-ID: IPiW70H5NPc

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Remove unused nsIFrameLoaderOwner::GetFrameLoaderXPCOM

There are no JS implementations of nsIFrameLoaderOwner, so we can mark it builtinclass.

MozReview-Commit-ID: 5z2f6fUrqaS

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Remove use of nsIFrameLoader from XPIDL files

MozReview-Commit-ID: 8pZ655SgrZ0

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 13. Remove nsIFrameLoader

MozReview-Commit-ID: 4LG8nIePsMH

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 14. Rename nsIFrameLoader.idl to nsIFrameLoaderOwner.idl

MozReview-Commit-ID: 1XpEjoFLSQh

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8957457 [details] [diff] [review]
part 1.  Remove unused stuff from nsIFrameLoader

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

This is my favourite kind of patch \o/

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8957458 [details] [diff] [review]
part 2.  Remove nsIFrameLoader::GetDocShell

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

::: dom/base/nsFrameLoader.cpp
@@ +619,5 @@
>    }
>    return CheckForRecursiveLoad(aURI);
>  }
>  
>  already_AddRefed<nsIDocShell>

Can we change this to produce a nsIDocShell instead of an already_AddRefed<nsIDocShell>? It'd be nice to avoid the unnecessary refcounting, and our nsFrameLoader will be keeping the docshell alive anyway.

::: layout/generic/nsSubDocumentFrame.cpp
@@ +1064,5 @@
>  }
>  
>  // XXX this should be called ObtainDocShell or something like that,
>  // to indicate that it could have side effects
> +already_AddRefed<nsIDocShell>

We should also be able to make this return a raw pointer, and avoid the unnecessary addref.

Comment 17

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8957462 [details] [diff] [review]
part 5.  Remove nsIFrameLoader::GetOwnerIsMozBrowserFrame

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

::: dom/base/nsFrameLoader.cpp
@@ +154,3 @@
>    NS_INTERFACE_MAP_ENTRY(nsIFrameLoader)
> +  if (aIID.Equals(NS_GET_IID(nsFrameLoader))) {
> +      foundInterface = static_cast<nsIFrameLoader*>(this);

We very explicitly want to have our returned pointer be indentical to our `this` pointer, so I think it might actually make the most sense to do `foundInterface = reinterpret_cast<nsISupports*>(this);`

At the very least we probably want to `MOZ_ASSERT(reinterpret_cast<nsFrameLoader*>(foundInterface) == this)` to validate that we did it right.

Comment 18

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8957463 [details] [diff] [review]
part 6.  Remove nsIFrameLoader::Get/SetEventMode

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

::: dom/base/nsFrameLoader.h
@@ +170,5 @@
>  
>    already_AddRefed<nsIMessageSender> GetMessageManager();
>  
>    uint32_t EventMode() const { return mEventMode; }
> +  void SetEventMode(uint32_t aEventMode);

Why not just make this inline?

Also, I popped open searchfox and couldn't find a single consumer of this setter (in JS or C++) - perhaps a follow-up to get rid of it completely?

Comment 19

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8957464 [details] [diff] [review]
part 7.  Remove nsIFrameLoader::Destroy

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

::: dom/webidl/FrameLoader.webidl
@@ +63,1 @@
>    void destroy();

Is this actually called by JS code? That would be pretty scary.

Comment 20

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8957465 [details] [diff] [review]
part 8.  Remove nsIFrameLoader::LoadFrame

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

::: dom/webidl/FrameLoader.webidl
@@ +42,1 @@
>    void loadFrame(optional boolean originalSrc = false);

Yeah - I think we should remove this API - I couldn't find any JS consumers either :-)

::: dom/xul/nsXULElement.cpp
@@ +1545,5 @@
>          // Usually xul elements are used in chrome, which doesn't have
>          // session history at all.
>          frameLoader = nsFrameLoader::Create(this, opener, false);
>          slots->mFrameLoaderOrOpener = static_cast<nsIFrameLoader*>(frameLoader);
> +        NS_ENSURE_TRUE_VOID(frameLoader);

Can you just switch this to an if (NS_WARN_IF(!frameLoader)) { return; } while you're here?

Comment 21

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8957466 [details] [diff] [review]
part 9.  Remove nsIFrameLoader::LoadURI

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

::: dom/webidl/FrameLoader.webidl
@@ +44,5 @@
>    /**
>     * Loads the specified URI in this frame. Behaves identically to loadFrame,
>     * except that this method allows specifying the URI to load.
>     */
> +  // XXXbz Should this even be exposed to JS?

I don't think this should be exposed to JS - can you remove it either here or in a follow-up?

Comment 22

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8957467 [details] [diff] [review]
part 10.  Remove unnecessary QIs to Ci.nsIFrameLoader in JS

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

*sheds a tear* It's beautiful

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 2 to produce nsIDocShell*

> Can we change this to produce a nsIDocShell instead of an already_AddRefed<nsIDocShell>?

Yes.  This interdiff does that, along with some cleanup to existing GetDocShell() callers to not store in an nsCOMPtr.

I renamed "docShell" to "parentDocShell" in nsFrameLoader::MaybeCreateDocShell for clarity, and I believe it caught a bug: the OwnerIsMozBrowserFrame bit was setting the name of the _parent_ docshell, which looks wrong to me.  I can change that back, of course, but I think the new behavior is clearly the one that code is aiming for.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 2 that actually compiles

Comment 25

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8957469 [details] [diff] [review]
part 12.  Remove use of nsIFrameLoader from XPIDL files

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

::: dom/base/nsFrameLoader.cpp
@@ +2549,5 @@
>    }
>  
> +  nsCOMPtr<nsISupports> otherLoaderAsSupports;
> +  browser->GetSameProcessAsFrameLoader(getter_AddRefs(otherLoaderAsSupports));
> +  RefPtr<nsFrameLoader> otherLoader = do_QueryObject(otherLoaderAsSupports);

This is a tad unfortunate :'-(.

Yet another reason for me to add WebIDL interface support to XPIDL ^_^

::: dom/base/nsIFrameLoader.idl
@@ +29,5 @@
>  native alreadyAddRefed_nsFrameLoader(already_AddRefed<nsFrameLoader>);
>  
> +// We define a "native" type for nsFrameLoader so that the rust xpidl
> +// codegen doesn't try to do anything with it.
> +native nativeFrameLoader(nsFrameLoader*);

Thanks! It's more common to write this as:

[ptr] native nativeFrameLoader(nsFrameLoader);

---

Another option here though is to just teach the rust XPIDL codegen what's going on by adding nsFrameLoader to nonidl.rs, and continue writing `interface`: https://searchfox.org/mozilla-central/rev/a70da6775d5341a9cca86bf1572a5e3534909153/xpcom/rust/xpcom/src/interfaces/nonidl.rs#53-55

@@ +35,2 @@
>  [scriptable, builtinclass, uuid(adc1b3ba-8deb-4943-8045-e6de0044f2ce)]
>  interface nsIFrameLoaderOwner : nsISupports

Looking forward to your patches to kill nsIFrameLoaderOwner ^_^

::: dom/browser-element/nsIBrowserElementAPI.idl
@@ +38,5 @@
>     * Notify frame scripts that support the API to destroy.
>     */
>    void destroyFrameScripts();
>  
> +  // The argument should be a FrameLoader.

Can you mention bug 1444991 here so it'll be easier to find places like this when that bug is fixed?

::: dom/interfaces/base/nsIBrowser.idl
@@ +15,5 @@
>     * browsers set this to the frame loader for the original content to ensure
>     * they are loaded in the same process as the content.
> +   *
> +   * This returns a FrameLoader, but we have no good way to represent
> +   * one in xpidl.

Can you mention bug 1444991 here so it'll be easier to find places like this when that bug is fixed?

::: dom/ipc/nsIHangReport.idl
@@ +65,5 @@
>    void endStartingDebugger();
>  
>    // Inquire whether the report is for a content process loaded by the given
>    // frameloader.
> +  bool isReportForBrowser(in nsISupports aFrameLoader);

mention bug 1444991 here too

::: dom/xul/nsXULElement.cpp
@@ +1581,5 @@
>  {
>      nsExtendedDOMSlots* slots = GetExistingExtendedDOMSlots();
>      MOZ_ASSERT(slots);
>  
> +    slots->mFrameLoaderOrOpener = ToSupports(aNewFrameLoader);

Feels like there should be a nicer way to implement this PresetOpenerWindow stuff (which unfortunately I wrote...) - it's pretty gross.

Comment 26

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8957470 [details] [diff] [review]
part 13.  Remove nsIFrameLoader

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

::: dom/base/nsFrameLoader.cpp
@@ +154,2 @@
>    if (aIID.Equals(NS_GET_IID(nsFrameLoader))) {
> +    foundInterface = ToSupports(this);

Again - we should at least assert that the ToSupports pointer is the same pointer as `this`

::: dom/base/nsFrameMessageManager.cpp
@@ +1027,5 @@
>  
>        if (aTargetFrameLoader) {
>          JS::Rooted<JS::Value> targetFrameLoaderv(cx);
> +        nsresult rv = nsContentUtils::WrapNative(cx, ToSupports(aTargetFrameLoader),
> +                                                 &targetFrameLoaderv);

Can we just use FrameLoaderBinding here instead of going through XPConnect?

::: dom/base/nsObjectLoadingContent.cpp
@@ +2576,5 @@
>  nsObjectLoadingContent::Traverse(nsObjectLoadingContent *tmp,
>                                   nsCycleCollectionTraversalCallback &cb)
>  {
>    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mFrameLoader");
> +  cb.NoteXPCOMChild(ToSupports(tmp->mFrameLoader));

Why not use CycleCollectionNoteChild here?

Comment 27

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8958184 [details] [diff] [review]
Interdiff for part 2 that actually compiles

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

::: dom/base/nsFrameLoader.cpp
@@ +2238,5 @@
>    if (OwnerIsMozBrowserFrame()) {
>      // For inproc frames, set the docshell properties.
>      nsAutoString name;
>      if (mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::name, name)) {
> +      mDocShell->SetName(name);

Yeah, I think this is the correct behaviour - thanks for fixing this ^_^

(Not that I think we have any named mozbrowser frames right now)

Comment 28

[Boris Zbarsky [:bzbarsky]]

> so I think it might actually make the most sense to do `foundInterface = reinterpret_cast<nsISupports*>(this);`

Good catch.  Done.

> Why not just make this inline?

Fair.  Done.

> perhaps a follow-up to get rid of it completely?

Bug 1445005.  Good catch; it looks like all the eventMode stuff is dead code.

> Is this actually called by JS code? That would be pretty scary.

Good point.  I will check and if not file a bug to remove it from the webidl.  (And if it is, I'll let you know why.  ;) )

> Yeah - I think we should remove this API - I couldn't find any JS consumers either :-)

OK.  I'm just going to do it in this patch.

> Can you just switch this to an if (NS_WARN_IF(!frameLoader)) { return; } while you're here?

Done.

> I don't think this should be exposed to JS - can you remove it either here or in a follow-up?

Bug 1445006 filed.  Searching for loadURI is ... hard, so removing it has some risk.

> This is a tad unfortunate :'-(.

Yeah.  The biggest issue is that it's _set_ from JS but has to be gotten from C++.  I couldn't find a cleaner way to do that.

> [ptr] native nativeFrameLoader(nsFrameLoader);

Good catch, fixed.

> by adding nsFrameLoader to nonidl.rs

Hmm.  That seems moderately fragile, with having to keep IIDs in sync.

> Looking forward to your patches to kill nsIFrameLoaderOwner ^_^

That one is a legit-ish interface with multiple implementations, so not immediately.

> Can you mention bug 1444991 here so it'll be easier to find places like this when that bug is fixed?
> Can you mention bug 1444991 here so it'll be easier to find places like this when that bug is fixed?
> mention bug 1444991 here too

Done, all three places.

> Feels like there should be a nicer way to implement this PresetOpenerWindow stuff 

Probably, but I decided to not mess with it here.  ;)

> Again - we should at least assert that the ToSupports pointer is the same pointer as `this`

I just left the reinterpet_cast<nsISupports*>.

> Can we just use FrameLoaderBinding here instead of going through XPConnect?

I switched to ToJSValue.  That will directly call GetOrCreateDOMReflector, etc.  No xpconnect, but also not directly FrameLoaderBinding, because it will check the wrappercache first.

> Why not use CycleCollectionNoteChild here?

Yeah, or even:

  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mFrameLoader);

which is what I just did.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Oh, the ToJSValue bit is:

        ok = ToJSValue(cx, aTargetFrameLoader, &targetFrameLoaderv);
        if (NS_WARN_IF(!ok)) {
          return NS_ERROR_UNEXPECTED;
        }

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd9aace7bb96
part 1.  Remove unused stuff from nsIFrameLoader.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1de8da1d7eb
part 2.  Remove nsIFrameLoader::GetDocShell.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/db30b43819c3
part 3.  Remove nsIFrameLoader::GetTabParent.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ad22190e537
part 4.  Remove nsIFrameLoader::GetDepthTooGreat.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/02ffc22a6b47
part 5.  Remove nsIFrameLoader::GetOwnerIsMozBrowserFrame.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/47433b671eb2
part 6.  Remove nsIFrameLoader::Get/SetEventMode.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/1695972ec057
part 7.  Remove nsIFrameLoader::Destroy.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/d53bbd945103
part 8.  Remove nsIFrameLoader::LoadFrame.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a965739c17
part 9.  Remove nsIFrameLoader::LoadURI.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7387120e7a1
part 10.  Remove unnecessary QIs to Ci.nsIFrameLoader in JS.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/f550087a772e
part 11.  Remove unused nsIFrameLoaderOwner::GetFrameLoaderXPCOM.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb84081046c5
part 12.  Remove use of nsIFrameLoader from XPIDL files.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/50328e36decd
part 13.  Remove nsIFrameLoader.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/31d28ee06481
part 14.  Rename nsIFrameLoader.idl to nsIFrameLoaderOwner.idl.  r=mystor

Comment 31

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/cd9aace7bb96
https://hg.mozilla.org/mozilla-central/rev/b1de8da1d7eb
https://hg.mozilla.org/mozilla-central/rev/db30b43819c3
https://hg.mozilla.org/mozilla-central/rev/3ad22190e537
https://hg.mozilla.org/mozilla-central/rev/02ffc22a6b47
https://hg.mozilla.org/mozilla-central/rev/47433b671eb2
https://hg.mozilla.org/mozilla-central/rev/1695972ec057
https://hg.mozilla.org/mozilla-central/rev/d53bbd945103
https://hg.mozilla.org/mozilla-central/rev/d5a965739c17
https://hg.mozilla.org/mozilla-central/rev/e7387120e7a1
https://hg.mozilla.org/mozilla-central/rev/f550087a772e
https://hg.mozilla.org/mozilla-central/rev/eb84081046c5
https://hg.mozilla.org/mozilla-central/rev/50328e36decd
https://hg.mozilla.org/mozilla-central/rev/31d28ee06481

Comment 32

[Jorg K (CEST = GMT+2)]

Boris, want's the replacement? See:
https://dxr.mozilla.org/comm-central/search?q=nsIFrameLoader&redirect=false

Comment 33

[Jorg K (CEST = GMT+2)]

Moving the discussion to bug 1447940.