Closed Bug 1657504 Opened 6 months ago Closed 6 months ago

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


(Core :: IPC, defect)




81 Branch
Tracking Status
firefox81 --- fixed


(Reporter: jld, Assigned: mccr8)




(3 files, 2 obsolete files)

Attached file ipdl-include-problem.diff (obsolete) —

STR: add a .ipdlh file to widget/, 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; 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.

Looking at the 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.

Assignee: nobody → continuation
Attached patch Exhibit the bug.Splinter Review
Attachment #9168305 - Attachment is obsolete: true

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 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.

Summary: Adding an IPDL source file can cause unrelated errors about manager/manages mismatches → IPDL parser allows a protocol to be defined in multiple files, leading to mysterious errors

This reverts commit 458de696e72cc41550fa8702883c9d734c915ecb.

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

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

Attachment #9168520 - Attachment is obsolete: true
Pushed by
part 1 - Don't allow a protocol to be defined in two different files. r=nika
part 2 - Fix deprecation warning in r=nika
See Also: → 1657712
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
You need to log in before you can comment on or make changes to this bug.