1375243 - Remove sync PHandlerService::Msg_Exists IPC message

Status: RESOLVED FIXED

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

Description

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

This seems to happen when loading the aljazeera page in the tp5o talos test with e10s enabled:

https://perf-html.io/public/e05b0638aa95a62ecfd046f71760abbf37be5c50/calltree/?invertCallstack&range=0.3706_0.7437~0.3730_0.7405~0.3918_0.5029&thread=1

Comment 1

[Andrew Overholt [:overholt]]

Blake, seems you reviewed this; can you take a look?

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

In my investigation here, I basically came to the same conclusion as Gijs did in bug 3375244. In order to fix the profile from comment 0, we should be avoiding the URI fixup code altogether for links from content.

That begin said, I do think it's relatively easy to fix Msg_Exists anyway, so I'll do that.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Blake Kaplan (:mrbkap) from comment #2)
> bug 3375244

bug 1375244

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Blake Kaplan (:mrbkap) from comment #2)
> That begin said, I do think it's relatively easy to fix Msg_Exists anyway,
> so I'll do that.

This is actually a bit more complicated than I thought. I misread the code the other day and now realize that this is checking two different databases of handlers. It's possible to fix this by shipping handler apps down to the child. I'll toy with a patch that tries that tomorrow and see what it looks like. I still think way to fix this is to avoid this stuff altogether.

Oh, and this case should be even more rare in the real world than this bug would suggest. This is happening because we rewrote our Tp5 suite to avoid network activity by rewriting all http: links as httpdisabled: links. That makes the fixup code think that this could actually be a mistyped URI, where I would wager that most <link href>s would be valid links (and would then skip this type of fixup).

    <link rel="alternate" type="application/rss+xml" title="الجزيرة نت" href="httpdisabled://www.aljazeera.net/AljazeeraRss/Rss.aspx?URL=RSS-portal.xml" />

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1375243 - Explicitly create a URI object to avoid sync URI fixups from the content process.

I'm not entirely certain that we need to pass the charset here, but it seems
like it might be needed based on my reading of the code.

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

Comment 6

[:Gijs (he/him)]

Comment on attachment 8895135 [details]
Bug 1375243 - Explicitly create a URI object to avoid sync URI fixups from the content process.

https://reviewboard.mozilla.org/r/166258/#review172168

Note that this will break showing RSS feeds with broken protocols that we used to fix up ("htp://foo.com/rss.xml") but I don't think we should care about that edgecase here.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Gijs from comment #6)
> Note that this will break showing RSS feeds with broken protocols that we
> used to fix up ("htp://foo.com/rss.xml") but I don't think we should care
> about that edgecase here.

To be clear, we never actually used the fixed-up URI to do anything. We'd do the security check on all of the default fixup variants (which included a few sync IPC calls) and then only ever add the invalid URI (which maps internally to an nsSimpleURI that we would be unable to load). This was purely wasted work.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99cf11868f47
Explicitly create a URI object to avoid sync URI fixups from the content process. r=Gijs

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=122399226&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/de7ef67a91478ceb71928e00c178a0f637050a98

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8895135 [details]
Bug 1375243 - Explicitly create a URI object to avoid sync URI fixups from the content process.

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

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Sorry about that. HTML elements must always have an ownerDocument (which will always have a characterSet). This test needs to mock its Link element more precisely.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87eaa504c2b5
Explicitly create a URI object to avoid sync URI fixups from the content process. r=Gijs

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87eaa504c2b5