1553957 - Add missing include of WindowProxyHolder.h (was incorrect: EventTarget.h needs to include WindowProxyHolder.h)

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Jorg K (CEST = GMT+2)]

Forward reference is not sufficient due to Nullable<WindowProxyHolder> GetOwnerGlobalForBindings();

That busts the Thunderbird compile.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1553957.patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9067172 [details] [diff] [review]
1553957.patch

Want to remove
template <typename>
 struct Nullable;

since Nullable.h is included anyhow

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1553957.patch

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/9a89e2b0fa9d
Include WindowProxyHolder.h in EventTarget.h. r=smaug a=Aryx

Comment 5

[Jorg K (CEST = GMT+2)]

Boris, in bug 1353867 comment #39 we already landed the same patch and reverted it later.

This time the compile errors were:
9:30.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/Maybe.h(158,51): error: invalid application of 'sizeof' to an incomplete type 'mozilla::dom::WindowProxyHolder'
9:30.01 MOZ_ALIGNAS_IN_STRUCT(T) unsigned char mStorage[sizeof(T)];
9:30.01 ^~~~~~~~~
9:30.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include/mozilla/dom/Nullable.h(26,12): note: in instantiation of template class 'mozilla::Maybe<mozilla::dom::WindowProxyHolder>' requested here
9:30.03 Maybe<T> mValue;

So I believe this not comes from Nullable<WindowProxyHolder> GetOwnerGlobalForBindings(); and the forward declaration isn't enough.

However, I've just noticed the it should have been #include "mozilla/dom/WindowProxyHolder.h", right?

Comment 6

[Boris Zbarsky [:bzbarsky]]

we already landed the same patch and reverted it later

Yes, because it wasn't needed to fix the compile... A much more targeted fix was fine.

This time the compile errors were:

What were all the errors, not just the last two lines? That information should have been attached to this bug (as an attachment; don't paste all that into a comment, please) when it was opened. It's really hard to say whether the fix in this bug is right given that you're withholding that information, for reasons I don't understand.

Comment 7

[Jorg K (CEST = GMT+2)]

Sorry, Boris. The error I quoted from Maybe.h seemed to imply that a forward reference was not enough any more. I think Olli dug through the include files in question and seemed to agree:

22:52:16 - smaug: but I don't know if there is any other fix

(or maybe I misunderstood).

I didn't intentionally withhold information, I'll provide the full compilation log with all the errors (as an attachment) in the course of the day (other stuff is burning hotter right now).

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1553957-complie-errors.txt

Here are the compile errors after backing out the fix locally. Search for Maybe.h or jump to line 377.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yeah, that's the same situation as in bug 1353867, exactly. The .cpp file involved is dom/ipc/JSWindowActorChild.cpp. It has:

Nullable<WindowProxyHolder> JSWindowActorChild::GetContentWindow(

so instantiates that template, and hence needs to include WindowProxyHolder.h, but does not. At risk of sounding like a broken record, let me just repeat bug 1353867 comment 49 verbatim:

The right answer is presumably for all the files that implement a function that returns Nullable<WindowProxyHolder> to include WindowProxyHolder.h. Some of them (e.g. docshell/base/BrowsingContext.cpp) are calling the WindowProxyHolder ctor, so really should be including that header anyway but aren't obviously doing so...

That's the right fix for this bug, not changing EventTarget.h.

I'll note that JSWindowActorChild.cpp is even calling the WindowProxyHolder constructor without including WindowProxyHolder.h, so it's clearly incorrect code.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, and that last bit is very clearly in the compile error log too, by the way:

17:43.13 c:/mozilla-source/comm-central/dom/ipc/JSWindowActorChild.cpp(121,12): error: 'mozilla::dom::WindowProxyHolder' is an incomplete type
17:43.13     return WindowProxyHolder(bc);
17:43.13            ^

Comment 11

[Jorg K (CEST = GMT+2)]

I'll fix it later today, afk right now. Should the

-template <typename>
-struct Nullable;

be restored given that mozilla/dom/Nullable.h is included?

Comment 12

[Boris Zbarsky [:bzbarsky]]

No, no need for that forward-decl if Nullable.h is included.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: Bug 1553957 - Follow-up: Revert changeset 9a89e2b0fa9d and include WindowProxyHolder.h in JSWindowActorChild.cpp.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: Follow-up.patch

Follow-up patch, same patch as in Phabricator.

Carrying over Boris' r+, hence r=bz.

Comment 15

[Jorg K (CEST = GMT+2)]

Dear Sheriff, please land the "follow-up" patch which can't be landed via Lando since it was uploaded via the Phab web UI.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3ce21755353
Follow-up: Revert changeset 9a89e2b0fa9d and include WindowProxyHolder.h in JSWindowActorChild.cpp. r=bzbarsky

Comment 17

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/a3ce21755353