1657504 - IPDL parser allows a protocol to be defined in multiple files, leading to mysterious errors

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: ipdl-include-problem.diff

STR: add a .ipdlh file to widget/moz.build, as in the attached diff; it doesn't need to be used anywhere and the name/contents don't seem to be important (but the file can't be completely empty, otherwise it's a parse error)

Result (edited slightly for Markdown compatibility; this seems to reproduce on Linux and (with a different file/line) Windows, but not Mac):

/home/jld/src/gecko-dev/widget/gtk/PCompositorWidget.ipdl:16: error: |manager| declaration in protocol 'PCompositorWidget' does not match any |manages| declaration in protocol 'PCompositorBridge'

The problem seems to be this code in type.py; if I replace the is tests in isManagerOf with an in like in isManagedBy, it works. This suggests that there are two objects representing one of the protocols, which compare unequal by object identity but equal by structural comparison.

Possibly this has something to do with the circular includes that are required for the manager/manages pair; we've had bugs around that in the past.

Comment 1

[Andrew McCreight [:mccr8]]

Looking at the moz.build file, I noticed that the Windows and Linux cases use different IPDL_SOURCES. I can reproduce on OSX by editing the IPDL_SOURCES to use the Windows case.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Exhibit the bug.

Comment 3

[Andrew McCreight [:mccr8]]

I think I figured out what is going on, guided by Jed's noticing that we probably had more than one instance of a protocol type, and the fact that it seemed to require some .ipdl files be in a subdirectory. We have two ways to parse a file. The first is that the file is passed in on the command line. The second is that you can do include protocol PCompositorWidget; and then it uses a method resolveIncludePath which tries to figure out what the file is. Anyways, in this particular case we have multiple files named PCompositorWidget.ipdl. The include resolution for PCompositorWidget.ipdl happens to find widget/PCompositorWidget.ipdl, but via the moz.build file, we specified widget/windows/PCompositorWidget.ipdl.

So, we parse the "plain" widget one first, and create a translation unit for it. The duplicate protection mechanism is based on the file name, not on the protocol name, so when we parse the "windows" widget file we don't find it. Then later we check the "windows" widget, but the protocol that it is managed by actually manages the other widget, and we get the type error.

I think the reason Jed's patch hits this error is that it placed an IPDL file in widget/, so that directory is now part of the include directories that are considered by resolveIncludePath, whereas they were not before.

I'm not sure what the best fix is here. Jed could probably hack around it by moving the stub .ipdl files in widget/ into a subdirectory like widget/stub/.

I have written a patch locally to change parsed to key on the protocol name instead of the file name, and instead check that the file name matches in the case of a cache hit, which gives a nice error in this situation, like "Found PCompositorWidget in file /Users/andrewmccreight/mc/widget/windows/PCompositorWidget.ipdl when we'd already seen it in file /Users/andrewmccreight/mc/widget/PCompositorWidget.ipdl".

I've long thought that the resolveIncludePath thing is hacky, and should not be needed for browser builds, so maybe we should ditch that and instead only consider the set of files that are explicitly passed in.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 1657504, part 1 - Don't allow a protocol to be defined in two different files.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Revert "Bug 1657504 - Exhibit the bug."

This reverts commit 458de696e72cc41550fa8702883c9d734c915ecb.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Bug 1657504, part 2 - Fix deprecation warning in ipdl.py.

This is unrelated to the main issue in the bug, but I noticed
a deprecation warning in the log spew.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3dd114ed0912
part 1 - Don't allow a protocol to be defined in two different files. r=nika
https://hg.mozilla.org/integration/autoland/rev/b5fde5227344
part 2 - Fix deprecation warning in ipdl.py. r=nika

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/3dd114ed0912
https://hg.mozilla.org/mozilla-central/rev/b5fde5227344