1322414 - Move content vs. content-primary distinction out of the `type` browser attribute

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[:Gijs (he/him)]

If we want to use type=chrome docshells to load chrome stuff (so we can prohibit loading chrome stuff in type=content docshells) then we need to be able to treat the chrome docshells as targetable/primary. In order to do that, either we have to introduce the targetable/primary distinction to type=chrome browsers, or we have to move that distinction to a separate attribute. The latter seems cleaner, so let's do that.

Comment 1

[Boris Zbarsky [:bzbarsky]]

See also bug 1310795, but it does look like we still use mPrimaryContentShell for a bunch of stuff...

Comment 2

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #1)
> See also bug 1310795, but it does look like we still use
> mPrimaryContentShell for a bunch of stuff...

I was looking at actually fixing this bug, and, uh, I'm confused. Because we read the attribute here and here:

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/base/nsFrameLoader.cpp#918
https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/base/nsFrameLoader.cpp#3271

and then pass it to contentshelladded:

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/base/nsFrameLoader.cpp#966
https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/base/nsFrameLoader.cpp#3291-3292

which AIUI ends up here:

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/xpfe/appshell/nsXULWindow.cpp#1701-1702,1708,1710-1713

which is odd because it uses "content-primary" and "content-targetable" as an ID? But all the content browsers have "content-targetable" except the active one... so I don't see how this is even working right now? What's the point of this code? AFAICT we don't have in-tree callers for GetContentShellById ( https://dxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsXULWindow.cpp#361 ) but it's still weeeeeird that we're doing this. :-\

Comment 3

[Boris Zbarsky [:bzbarsky]]

I was just looking at the same thing while working on bug 1310796.  I think the whole id thing looks pretty bogus to me and we should remove it.

We do have an in-tree caller of GetContentShellById, though: nsWebShellWindow::LoadContentAreas.  I'm not sure whether it's ever reached in practice with a URL that has the right bits in it to trigger that codepath.

Comment 4

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3)
> We do have an in-tree caller of GetContentShellById, though:
> nsWebShellWindow::LoadContentAreas.  I'm not sure whether it's ever reached
> in practice with a URL that has the right bits in it to trigger that
> codepath.

I don't know of any chrome that does that, and the code seems to have been dead for 15 years. So I assume we can just nuke it, but here's a trypush before I go any further, I guess:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=2df941111da33628bb1fc01de1b7bd1c39055fd6

Comment 5

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b195e2fb3b276aa6a030be62dac37d4b5a7e74d

Comment 6

[:Gijs (he/him)]

Attachment: Bug 1322414 - part 1 - remove GetContentShellById and id passing,

Review commit: https://reviewboard.mozilla.org/r/97856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97856/

Comment 7

[:Gijs (he/him)]

Attachment: Bug 1322414 - part 2: use a separate 'primary' attribute for primary child browsers,

Review commit: https://reviewboard.mozilla.org/r/97858/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97858/

Comment 8

[:Gijs (he/him)]

Attachment: Bug 1322414 - part 3: use primary attribute in frontend,

Review commit: https://reviewboard.mozilla.org/r/97860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97860/

Comment 9

[:Gijs (he/him)]

Attachment: Bug 1322414 - part 4: fix test usage of type=content-primary to use type=content and primary=true,

Review commit: https://reviewboard.mozilla.org/r/97862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97862/

Comment 10

[:Gijs (he/him)]

Boris, would you mind reviewing the first two parts, as you've just touched the same code in bug 1310796? :-)

Comment 11

[:Gijs (he/him)]

Annnd there's quite a bit of orange - it seems that window.content is null in some circumstances on non-e10s. I expect I missed something somewhere...

Comment 12

[:Gijs (he/him)]

Comment on attachment 8817639 [details]
Bug 1322414 - part 2: use a separate 'primary' attribute for primary child browsers,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97858/diff/1-2/

Comment 13

[:Gijs (he/him)]

Comment on attachment 8817640 [details]
Bug 1322414 - part 3: use primary attribute in frontend,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97860/diff/1-2/

Comment 14

[:Gijs (he/him)]

Comment on attachment 8817641 [details]
Bug 1322414 - part 4: fix test usage of type=content-primary to use type=content and primary=true,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97862/diff/1-2/

Comment 15

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #11)
> Annnd there's quite a bit of orange - it seems that window.content is null
> in some circumstances on non-e10s. I expect I missed something somewhere...

I forgot to update the mutation observer to care about updates to the 'primary' attribute. Oops.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68681056de2f

Comment 16

[:Gijs (he/him)]

(the remaining orange is because of my previous patch for bug 1322609 being broken)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8817638 [details]
Bug 1322414 - part 1 - remove GetContentShellById and id passing,

https://reviewboard.mozilla.org/r/97856/#review98386

So after this change, doing ContentShellAdded for non-primary things is just a really fancy and obfuscated way of setting their treeowner, eh?  At least if we ignore the fact that we can have someone else implementing a treeowner....

::: xpfe/appshell/nsWebShellWindow.cpp
(Diff revision 1)
> -        AppendUTF8toUTF16(search, searchSpec);
> -      }
> -    }
> -  }
> -
> -  // content URLs are specified in the search part of the URL

I'm hoping we know for a fact that this is dead code.  I guess this is for a _chrome_ window url with a query string like that?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8817639 [details]
Bug 1322414 - part 2: use a separate 'primary' attribute for primary child browsers,

https://reviewboard.mozilla.org/r/97858/#review98388

Presumably you'll merge parts 3 and 4 in here, so we don't have the temporary broken state?

::: dom/base/nsFrameLoader.cpp:917
(Diff revision 2)
>  {
>    NS_PRECONDITION(aItem, "Must have docshell treeitem");
>    NS_PRECONDITION(mOwnerContent, "Must have owning content");
>  
>    nsAutoString value;
> -  bool isContent = false;
> +  bool isContent = mOwnerContent->AttrValueIs(

This might break extensions that use "content-something" values, no?  Do we know for a fact that this doesn't happen?

Yes, I should have restricted this to "content-targetable" and "content-primary" back when.  :(

I guess I'm OK with this change as long as we loop Jorge in on it.

Comment 19

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #17)
> Comment on attachment 8817638 [details]
> Bug 1322414 - part 1 - remove GetContentShellById and id passing,
> 
> https://reviewboard.mozilla.org/r/97856/#review98386
> 
> So after this change, doing ContentShellAdded for non-primary things is just
> a really fancy and obfuscated way of setting their treeowner, eh?  At least
> if we ignore the fact that we can have someone else implementing a
> treeowner....

Yes, and ContentShellRemoved + ContentShellAdded is mostly obfuscated 'type' and primary switching. It probably makes sense to have 2-3 separate methods to do these 2-3 separate things. I'll file a followup bug if/when this stuff lands successfully...

> ::: xpfe/appshell/nsWebShellWindow.cpp
> (Diff revision 1)
> > -        AppendUTF8toUTF16(search, searchSpec);
> > -      }
> > -    }
> > -  }
> > -
> > -  // content URLs are specified in the search part of the URL
> 
> I'm hoping we know for a fact that this is dead code.

That's hard to do, given that URLs are basically just random strings in (as well as outside!) our codebase. :-\

Basically, the code here hasn't changed since before 2003, besides refactorings like nsbool vs bool and the like, which seems like a reasonable indication that nobody actually uses it - but perhaps my impression is coloured by the 'recency' of what I usually work on?

The other indication that this is dead is that, as far as I understand, for it to work, there'd need to be code that has type=content-<unique-identifier> frames. I looked for such frames in m-c (basically, used dxr to search for lines that contained both 'type' and 'content-', and then filtered out 'content-length', 'content-type' etc.) and didn't find any.

>  I guess this is for a
> _chrome_ window url with a query string like that?

As far as I can tell, yes.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8817640 [details]
Bug 1322414 - part 3: use primary attribute in frontend,

https://reviewboard.mozilla.org/r/97860/#review98652

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8817641 [details]
Bug 1322414 - part 4: fix test usage of type=content-primary to use type=content and primary=true,

https://reviewboard.mozilla.org/r/97862/#review98662

This all looks okay to me - though I didn't do a searchfox/dxr to make sure you hit all of the spots. I assume that automation will catch that.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6a73f37649
part 1 - remove GetContentShellById and id passing, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3c689dbcd92
part 2,3,4: use a separate 'primary' attribute for primary child browsers, r=bz,mconley

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9a6a73f37649
https://hg.mozilla.org/mozilla-central/rev/e3c689dbcd92

Comment 24

[Jorg K (CEST = GMT+2)]

Hmm, that completely smashed Thunderbird without notice so we fixed it here:
https://hg.mozilla.org/comm-central/rev/42aa2df379ce3ffa520f01c2799a9e4283c45327

We changed type="content-targetable" to type="content" and type="content-primary" to type="content" and primary="true".

What about "content-conversation"?
http://searchfox.org/comm-central/search?q=content-conversation&case=true&regexp=false&path=

Comment 25

[:Gijs (he/him)]

(In reply to Jorg K (GMT+1) from comment #24)
> Hmm, that completely smashed Thunderbird without notice so we fixed it here:
> https://hg.mozilla.org/comm-central/rev/
> 42aa2df379ce3ffa520f01c2799a9e4283c45327
> 
> We changed type="content-targetable" to type="content" and
> type="content-primary" to type="content" and primary="true".
> 
> What about "content-conversation"?
> http://searchfox.org/comm-central/search?q=content-
> conversation&case=true&regexp=false&path=

It wants to be type="content". If it was never set to type="content-primary" before, I suspect that's all you need to do.

Comment 26

[:aceman]

(In reply to :Gijs Kruitbosch from comment #15)
> (In reply to :Gijs Kruitbosch from comment #11)
> > Annnd there's quite a bit of orange - it seems that window.content is null
> > in some circumstances on non-e10s. I expect I missed something somewhere...
> 
> I forgot to update the mutation observer to care about updates to the
> 'primary' attribute. Oops.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=68681056de2f

Hi, what is the plan to fix this? Have you made a followup bug?

Comment 27

[:Gijs (he/him)]

(In reply to :aceman from comment #26)
> (In reply to :Gijs Kruitbosch from comment #15)
> > (In reply to :Gijs Kruitbosch from comment #11)
> > > Annnd there's quite a bit of orange - it seems that window.content is null
> > > in some circumstances on non-e10s. I expect I missed something somewhere...
> > 
> > I forgot to update the mutation observer to care about updates to the
> > 'primary' attribute. Oops.
> > 
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=68681056de2f
> 
> Hi, what is the plan to fix this? Have you made a followup bug?

This was on try. I updated the patches and only the updated patches landed (and were green, or they would have been backed out already).

Comment 28

[Jarda Snajdr [:jsnajdr]]

This change breaks the RDP Inspector addon - see this issue:

https://github.com/firebug/rdp-inspector/issues/88

It's bad that there is no way to set type="content-primary" in a way that works both in 52 and 53. The RDP Inspector seems to work well even with primary="false". But I'm not 100% sure about that and maybe there are other addons that really require primary="true".

Comment 29

[:Gijs (he/him)]

(In reply to Jarda Snajdr [:jsnajdr] from comment #28)
> This change breaks the RDP Inspector addon - see this issue:
> 
> https://github.com/firebug/rdp-inspector/issues/88
> 
> It's bad that there is no way to set type="content-primary" in a way that
> works both in 52 and 53. The RDP Inspector seems to work well even with
> primary="false". But I'm not 100% sure about that and maybe there are other
> addons that really require primary="true".

It can detect whether these changes are present on a build it's running in using the presence or lack thereof of the getContentShellById API on nsIXULWindow.

You only need the primary/non-primary distinction if you've got more than one child content docshell in your window, which from a quick look your add-on doesn't seem to. If you did have more than 1 kid docshell you'd presumably change the 'primary' at runtime anyway, so the extra check in JS is relatively easy to implement.

That said, it looks like content-primary is used relatively frequently by add-ons (711 hits on DXR). I don't really understand why, several of the cases where add-ons check for those values rather than using them themselves look like broken ways of checking "is this a main firefox tabbrowser browser", but yeah...

I'm not sure how much we should care given XUL add-ons are going away and the distance to release for this change, plus the fact that all the add-ons I've spot-checked so far are unmaintained. I'm going to need time I don't have right now to go through the list properly...

Unfortunately the checks for these attributes are more spread out than one might like, and so I would prefer not to support both syntaxes if we can help it, to avoid the code becoming hairier than it needs to be.

I could add support for "content-*" meaning "content" again, but it's also unclear how many of the add-ons that would fix.

I'll come back to this when my request queue is less depressing. Meanwhile, Boris, thoughts about the above and whether we *need* to re-add some complexity to support both syntaxes?

(Clearly, based on comment #18, I should have looked at this before - not sure why that fell through the cracks, but my apologies either way... :-( )

Comment 30

[Boris Zbarsky [:bzbarsky]]

Did we ever ping Jorge about this?

I do think it makes sense to support "content-*" meaning "content" for a bit while people migrate.  It would allow <browser type="content-primary" primary="true"> as a transitional thing.  I can't speak to the support status for XUL addons and timeframes; I haven't really been tracking that.

The addons that are _checking_ for content-primary are a bigger problem; those probably just need to be updated.

Comment 31

[:Gijs (he/him)]

I've put up a patch in the dep.

Comment 32

[Jorge Villalobos [:jorgev] (he/him)]

Supporting "content-*" as "content" at least for a couple of cycles would be ideal, I agree.