1318110 - Incorrect handling of errors in nested IPDL parser

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

I recently tried to add the ClientType enum import to an ipdlh file:

========================
/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

namespace mozilla {
namespace dom {

using mozilla::dom::ClientType from "ClientType.h"

struct ClientHandleConstructorArgs
{
  bool todo;
};

struct ClientSourceConstructorArgs
{
  mozilla::dom::ClientType type;
};

} // namespace dom
} // namespace mozilla
=============================

In return the ipdl compiler gave me:

 0:06.32   in file included from `/srv/mozilla-central/dom/clients/PClientManager.ipdl', line 8:
 0:06.32 /srv/mozilla-central/dom/clients/ClientIPCTypes.ipdlh:46: error: can't define a protocol in a header.  Do it in a protocol spec instead.
 0:06.32 Specification could not be parsed.


This error message is bad for a few reasons:

1. There are not 46 lines in ClientIPCTypes.ipdlh.  It only has 21 lines.
2. I'm not trying to introduce a protocol.
3. It doesn't tell me what symbol its choking on.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Andrew, you asked me to file a bug for bad ipdl compiler error messages.  Here is one I just ran into.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

The end problem here was that this line:

  using mozilla::dom::ClientType from "ClientType.h"

Referenced a header it couldn't find in other directories.  After I exported the header and changed it to "mozilla/dom/ClientType.h" then it compiled.

The error message really doesn't help, though.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #2)
> The end problem here was that this line:
> 
>   using mozilla::dom::ClientType from "ClientType.h"
> 
> Referenced a header it couldn't find in other directories.  After I exported
> the header and changed it to "mozilla/dom/ClientType.h" then it compiled.
> 
> The error message really doesn't help, though.

Actually, its still failing.  Not sure what the problem is. :-\

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Turns out the problem was that the line:

  using mozilla::dom::ClientType from "ClientType.h"

Was inside the namespace in the ipdlh file.

Comment 5

[Andrew McCreight [:mccr8]]

That's odd. Putting "using" inside "namespace" should be a parser error.

Comment 6

[Andrew McCreight [:mccr8]]

When I created a new file as given in comment 0, I did get a parser error:
 0:08.27 /home/amccreight/mc/dom/ipc/Foo.ipdlh:8: error: bad syntax near `using'
 0:08.27 Specification could not be parsed.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Example files.

Comment 8

[Andrew McCreight [:mccr8]]

Ben, what were you doing differently from what I have in the patch? I'd like to figure out how it was triggering that error. Thanks.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I believe the error was from when the ipdlh was being included from an ipdl file.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: Example files.

That did the trick. Thanks.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: Bug 1318110 - Correctly handle parse errors in inner IPDL parsers.

The IPDL parser handles include statements by using a stack of
parsers. If an inner parser encounters a parsing error, it will print
out an error message, which is maybe okay, but then it makes two
mistakes:

1. It does not pop the current parser off of the parser stack. This
means that the file that included the syntactically invalid file will
be parsed as though it were the invalid file. In bkelly's case, an
.ipdl file included an invalid .ipdlh file, so he got a bizarre error
message about how you can't define a protocol in a header, because the
parser was treating the protocol in the .ipdl file as though it were
in the .ipdlh file. I fixed this by using a "finally" clause to pop
the parser stack, ensuring that it is correct even in case of error.

2. A parse error in the include should cause the entire parse to fail,
but instead it will keep going. inc.tu will get set to None, which
eventually causes an error later in type checking, when it attempts to
examine inc.tu. I fixed this by only catching the parse error where we
invoke the outermost parser. This has the drawback that later errors
in other files will not be reported. An alternate fix would set a
global flag to indicate that a parse error had occured, and somehow
report that to the caller.

I think this bug was introduced in 2009 by commit
cb8189926a69872c73508fba50830f0d07af341f.

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

Comment 12

[Andrew McCreight [:mccr8]]

There's details in the commit message, but basically handling parser errors inside an included file was totally broken, and has been since 2009.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8811954 [details]
Bug 1318110 - Correctly handle parse errors in inner IPDL parsers.

https://reviewboard.mozilla.org/r/93846/#review94044

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1156c54cdb68
Correctly handle parse errors in inner IPDL parsers. r=billm

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1156c54cdb68

Comment 16

[areya]

https://hg.mozilla.org/mozilla-central/rev/1156c54cdb68