Remove sync PHandlerService::Msg_Exists IPC message

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: mconley, Assigned: mrbkap)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment)

Reporter

Updated

2 years ago
Blocks: SyncIPC
Whiteboard: [qf]

Updated

2 years ago
Whiteboard: [qf] → [qf:p1]
Blake, seems you reviewed this; can you take a look?
Flags: needinfo?(mrbkap)
Priority: -- → P1
Assignee: nobody → mrbkap
Flags: needinfo?(mrbkap)
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.
Depends on: 1375244
(In reply to Blake Kaplan (:mrbkap) from comment #2)
> bug 3375244

bug 1375244
(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 hidden (mozreview-request)
Attachment #8895135 - Flags: review?(felipc) → review?(gijskruitbosch+bugs)

Comment 6

2 years ago
mozreview-review
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.
Attachment #8895135 - Flags: review?(gijskruitbosch+bugs) → review+
(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

2 years ago
Pushed by mrbkap@mozilla.com:
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 hidden (mozreview-request)
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.
Flags: needinfo?(mrbkap)

Comment 12

2 years ago
Pushed by mrbkap@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/87eaa504c2b5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.