1353867 - Implement Cross-Process async window proxies

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Nika Layzell [:nika] (ni? for response)]

Two windows which aren't part of the same Unit of Related Similar-Origin Browsing Contexts (say google.com with a window.opener which refers to a window currently opened to apple.com), cannot communicate with each other synchronously. This means that it should be possible for us to maintain the same API as we do today (where the two windows have window references to each other) while placing the two tabs in different processes. This would require support for some form of cross-process async window wrapper object.

This would be a large project, as there are a decent number of places in our internal code where we expect full window objects to be avaliable. We would have to modify these locations to work correctly in the case where the target of the operation is cross process. 

One way which this could be implemented would be similar to the following:
* All window proxy objects held by content are replaced with a `dom::WindowProxy` object which itself does not hold the WindowProxy state but implements every method which would be expected to be implemented on a Window object.
* Other objects which would need so support async methods should have proxies too (such as dom::LocationProxy for the window.location object)
* Methods on the WindowProxy object are handled by talking to a process-local WindowProxyManager class which will look up the window object corresponding to the WindowProxy object. 
* If the object exists within the current process, all methods will be supported, and the request will be directly, synchronously, forwarded to the correct Window object.
* If the window doesn't exist within the current process, any calls to synchronous methods will assert (content code should be prevented by principal checks from calling one of these methods anyway), and calls to async methods will be forwarded over IPC to the correct process.
* When a window navigates, if the navigation would require changing process (for example, because the window is now becoming same-origin with another window in it's unit of related browsing contexts), then a new window object will be created, and the WindowProxyManagers will have their state updated to understand which process the window in question is currently in.
* The current shape of the window tree (such as iframe count etc.) will be asynchronously synchronized between processes for the purposes of implementing properties on WindowProxy objects synchronously such as window.frames and window.parent.

Unfortunately this feature has limited value without the ability to move iframes out of process, which would be necessary to have two related windows which are in different tabs not have the possibility of breaking. This is, however, also a requirement for the implementation of that feature, and a great step in that direction if we succeed in implementing it.

Comment 1

[Andrew Overholt [:overholt]]

(In reply to Michael Layzell [:mystor] from comment #0)
> Unfortunately this feature has limited value without the ability to move
> iframes out of process, which would be necessary to have two related windows
> which are in different tabs not have the possibility of breaking. This is,
> however, also a requirement for the implementation of that feature, and a
> great step in that direction if we succeed in implementing it.

This makes me think P3 at best :)

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrew Overholt [:overholt] from comment #1)
> This makes me think P3 at best :)

I agree - potentially even lower. I mostly made this bug as a place to dump my thoughts about this. I doubt we'll be working on it for a while.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

I occasionally think about how this will be accomplished on the bus to work - so dumping more ideas about what the process of implementing this would look like:

1) Finish khuey's work on splitting Inner/Outer windows into two separate classes
2) Merge the outer window into nsDocShell.
3) Add a class nsDocShellProxy which exposes a similar interface to nsDocShell, but proxies calls to its methods through a global nsDocShellManager object. Principal / permissions checks for calls to methods on window proxies can probably be moved into nsDocShellManager.
4) Modify the DocShell tree to be a tree of nsDocShellProxy objects instead of nsDocShell objects.
5) Cache information which can be synchronously accessed by cross-origin JS code in the nsDocShellManager, and use asynchronous event dispatch to synchronize the information.
6) Teach nsDocShellProxy how to asynchronously call methods such as postMessage on nsDocShell objects which are in a different process.
7) Synchronize docshell trees across processes, and ensure that you can call methods on out of process docshells.
8) Modify all chrome framescripts to not expect the ability to synchronously see across origin boundaries.

This is a lot of work - We'd probably have to break it down more than this whenever we want to do this. Once we get to this point we should be able to start talking about moving each DocGroup into its own process.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

We might be able to get useful results out of cross process window proxy objects if we have first party isolation enabled. (see also bug 1425287, which might also be an option).

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(I'm sorry I talk about the clients stuff I wrote a lot, but it might be able to help here as well.)

Right now we can reference a window in one process from another process via ClientInfo token.  This can be used to create a ClientHandle which is an active attachment using IPC actors.  This can be used to postMessage() and asynchronously capture some state information about that window today.

I've had plans in my mind to make things like SharedWorker and nested workers use ClientInfo/ClientHandle for their parent.  It seems to me we might be able to do the same for cross-origin iframes.

Looking at comment 3:

* ClientHandle has the vast majority of (6).  We would just need to tweak the event target for the postMessage.
* We could adapt the ClientState information to include the information needed for (5).

All the stuff about integrating with docshell, though, probably needs to be done one way or another.  If we used ClientHandle here it would mean wrapping it in some kind of docshell adapter or making docshell understand ClientHandle directly.

Another thing we probably need is teardown code.  If the parent is represented as a ClientHandle, then you can use its OnDetach() MozPromise to cleanup the out-of-process iframe when the parent goes away.  We would still need some other call to handle things like removing the frame from the DOM via .remove(), etc.

Anyway, just throwing that out there.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Thanks for doing this work bkelly :-). Im pretty sure that whatever we end up doing for cross process window proxies will be built on top of the client work you've already done.

I've mostly been thinking about how to implement the edges of the window proxy work (aka how to change nsGlobalWindowOuter and nsDocShell to enable this). Your comment is going to be super handy for filling in the actual connections :-).

Comment 7

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b3970f7670246f6ae377d7498315a56caf459d0

Comment 8

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb3d1fdda9e435d267122e20c7d6f00f36643eaa

Comment 9

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88f1a6865995d050322be3f9b0b1b845466ed93e

Comment 10

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24578b3ac0c05dc69120af76aa8e458389577c3d

Comment 11

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b83310c889edda6f076adc1f90a5c1034940317b

Comment 12

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=669051bfdb337d9302069a99185316b9d9083be1

Comment 13

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1eb6c8a4b478e1009903b5bad40c859fff7eedd7

Comment 14

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ab6ffe0746fdf5c26692fdf95f12893d86c2fa9

Comment 15

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=549123532831711c6871407d0c0a3d357f006824

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: Bug 1353867 - Change WindowProxy type. r=bzbarsky

Add a WindowProxyHolder type and generate binding code that takes or returns it whenever
the WebIDL refers to the WindowProxy type. This patch just makes the WindowProxyHolder
hold a strong reference to a nsPIDOMWindowOuter.

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: Bug 1353867 - Change WindowProxyHolder's native type to BrowsingContext. r=bzbarsky

Make the WindowProxyHolder hold a strong reference to a BrowsingContext, as in the future
we might not have a nsPIDOMWindowOuter (if the document is loaded in a different process).

Depends on D12650

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: Bug 1353867 - Split up PostMessage into two parts, one gathering information from the source and one gathering information from the target. r=bzbarsky

Depends on D12651

Comment 19

[Peter Van der Beken [:peterv]]

Attachment: Bug 1353867 - Add code generation for array of cross origin properties. r=bzbarsky

Depends on D12652

Comment 20

[Peter Van der Beken [:peterv]]

Attachment: Bug 1353867 - Expose IsCrossOriginWhitelistedProp/AppendCrossOriginWhitelistedPropNames to DOM code. r=bzbarsky

Depends on D12654

Comment 21

[Peter Van der Beken [:peterv]]

Attachment: Bug 1353867 - Add cross-process proxies for WindowProxy. r=bzbarsky

Depends on D12655

Comment 22

[Peter Van der Beken [:peterv]]

Attachment: Bug 1353867 - Add cross-process proxies for Location. r?bzbarsky!

Depends on D12656

Comment 23

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=239319833dc0d9bd75f8abf0457f86d615c35b15

Comment 24

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7de56c7495b1a60276899c9aa37f6f6aba6024c

Comment 25

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63b692d61c6921ff63e7f48b63c25dc830b579cb

Comment 26

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59730f893510362a71e8b2e56966f638acb8e1e8

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ebdf4531b3bd
Change WindowProxy type. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/8b60851b93da
Change WindowProxyHolder's native type to BrowsingContext. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/700aeb906fd2
Split up PostMessage into two parts, one gathering information from the source and one gathering information from the target. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/00869bed4121
Add code generation for array of cross origin properties. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/8c05f4d3f7ad
Expose IsCrossOriginWhitelistedProp/AppendCrossOriginWhitelistedPropNames to DOM code. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/aa9b106b15d9
Add cross-process proxies for WindowProxy. r=bzbarsky

Comment 28

[Peter Van der Beken [:peterv]]

The location patch still needs final review and landing.

Comment 29

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0d91849ccb91bf99f21a4fe48bb4cd41d0600a0

Comment 30

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 6 changesets (Bug 1353867) for nsDocShell.cpp failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&collapsedPushes=418327%2C418336%2C418345%2C418367%2C418384&selectedJob=219402511&revision=aa9b106b15d93573e5125356667f3a1f474d2e5a

Backout link: https://hg.mozilla.org/integration/autoland/rev/cc4bb8c7fa92e8a4f44377c253160cea4ab66c97

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=219402511&repo=autoland&lineNumber=5903

[task 2018-12-31T15:19:14.296Z] 15:19:14     INFO - TEST-START | dom/browser-element/mochitest/test_browserElement_inproc_BrowserWindowNamespace.html
[task 2018-12-31T15:19:14.399Z] 15:19:14     INFO - GECKO(2775) | ++DOMWINDOW == 69 (0xdd590800) [pid = 2775] [serial = 69] [outer = 0xde170eb0]
[task 2018-12-31T15:19:14.569Z] 15:19:14     INFO - GECKO(2775) | ++DOCSHELL 0xdc194400 == 18 [pid = 2775] [id = {3d35d450-2ece-4ba1-a678-d25507e9f03c}]
[task 2018-12-31T15:19:14.571Z] 15:19:14     INFO - GECKO(2775) | ++DOMWINDOW == 70 (0xddf33430) [pid = 2775] [serial = 70] [outer = (nil)]
[task 2018-12-31T15:19:14.588Z] 15:19:14     INFO - GECKO(2775) | ++DOMWINDOW == 71 (0xdc196000) [pid = 2775] [serial = 71] [outer = 0xddf33430]
[task 2018-12-31T15:19:14.746Z] 15:19:14     INFO - GECKO(2775) | ++DOMWINDOW == 72 (0xdd59a800) [pid = 2775] [serial = 72] [outer = 0xddf33430]
[task 2018-12-31T15:19:14.863Z] 15:19:14     INFO - GECKO(2775) | ++DOCSHELL 0xdd599400 == 19 [pid = 2775] [id = {21c522e1-0373-4cc5-b3fa-a43f1329ddec}]
[task 2018-12-31T15:19:14.863Z] 15:19:14     INFO - GECKO(2775) | ++DOMWINDOW == 73 (0xddf336d0) [pid = 2775] [serial = 73] [outer = (nil)]
[task 2018-12-31T15:19:14.869Z] 15:19:14     INFO - GECKO(2775) | ++DOMWINDOW == 74 (0xdd59c800) [pid = 2775] [serial = 74] [outer = 0xddf336d0]
[task 2018-12-31T15:19:14.976Z] 15:19:14     INFO - GECKO(2775) | [2775, Main Thread] WARNING: '!mSelection', file /builds/worker/workspace/build/src/editor/libeditor/EditorBase.cpp, line 4813
[task 2018-12-31T15:19:14.977Z] 15:19:14     INFO - GECKO(2775) | [2775, Main Thread] WARNING: '!editActionData.CanHandle()', file /builds/worker/workspace/build/src/editor/libeditor/EditorBase.cpp, line 1250
[task 2018-12-31T15:19:14.997Z] 15:19:14     INFO - GECKO(2775) | ++DOMWINDOW == 75 (0xdda06400) [pid = 2775] [serial = 75] [outer = 0xddf336d0]
[task 2018-12-31T15:19:15.034Z] 15:19:15     INFO - GECKO(2775) | Assertion failure: !mOriginAttributes.mInIsolatedMozBrowser || (GetInheritedFrameType() == FRAME_TYPE_BROWSER) (Isolated mozbrowser should only be true inside browser frames), at /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp:12966
[task 2018-12-31T15:20:45.770Z] 15:20:45     INFO - GECKO(2775) | #01: nsDocShell::GetIsInIsolatedMozBrowserElement(bool*) [mfbt/Assertions.h:38]
[task 2018-12-31T15:20:45.770Z] 15:20:45     INFO - 
[task 2018-12-31T15:20:45.770Z] 15:20:45     INFO - GECKO(2775) | #02: non-virtual thunk to nsDocShell::GetIsInIsolatedMozBrowserElement(bool*) [docshell/base/nsDocShell.cpp:0]
[task 2018-12-31T15:20:45.772Z] 15:20:45     INFO - 
[task 2018-12-31T15:20:45.774Z] 15:20:45     INFO - GECKO(2775) | #03: NS_CompareLoadInfoAndLoadContext(nsIChannel*) [netwerk/base/nsNetUtil.cpp:2739]
[task 2018-12-31T15:20:45.776Z] 15:20:45     INFO - 
[task 2018-12-31T15:20:45.777Z] 15:20:45     INFO - GECKO(2775) | #04: mozilla::net::nsHttpChannel::AsyncOpen(nsIStreamListener*, nsISupports*) [netwerk/protocol/http/nsHttpChannel.cpp:6035]
[task 2018-12-31T15:20:45.779Z] 15:20:45     INFO - 
[task 2018-12-31T15:20:45.781Z] 15:20:45     INFO - GECKO(2775) | #05: mozilla::net::nsHttpChannel::AsyncOpen2(nsIStreamListener*) [netwerk/protocol/http/nsHttpChannel.cpp:6202]
[task 2018-12-31T15:20:45.782Z] 15:20:45     INFO - 
[task 2018-12-31T15:20:45.784Z] 15:20:45     INFO - GECKO(2775) | #06: non-virtual thunk to mozilla::net::nsHttpChannel::AsyncOpen2(nsIStreamListener*) [netwerk/protocol/http/nsHttpChannel.cpp:0]
[task 2018-12-31T15:20:45.785Z] 15:20:45     INFO - 
[task 2018-12-31T15:20:45.786Z] 15:20:45     INFO - GECKO(2775) | #07: nsURILoader::OpenURI(nsIChannel*, unsigned int, nsIInterfaceRequestor*) [uriloader/base/nsURILoader.cpp:837]
[task 2018-12-31T15:20:45.787Z] 15:20:45     INFO - 
[task 2018-12-31T15:20:45.787Z] 15:20:45     INFO - GECKO(2775) | #08: nsDocShell::DoChannelLoad(nsIChannel*, nsIURILoader*, bool) [docshell/base/nsDocShell.cpp:10479]
[task 2018-12-31T15:20:45.788Z] 15:20:45     INFO - 
[task 2018-12-31T15:20:45.788Z] 15:20:45     INFO - GECKO(2775) | #09: nsDocShell::DoURILoad(nsDocShellLoadState*, bool, nsIDocShell**, nsIRequest**, nsTSubstring<char16_t> const&, unsigned int) [docshell/base/nsDocShell.cpp:10278]
[task 2018-12-31T15:20:45.789Z] 15:20:45     INFO - 
[task 2018-12-31T15:20:45.790Z] 15:20:45     INFO - GECKO(2775) | #10: nsDocShell::InternalLoad(nsDocShellLoadState*, nsIDocShell**, nsIRequest**) [docshell/base/nsDocShell.cpp:9599]
[task 2018-12-31T15:20:45.791Z] 15:20:45     INFO - 
[task 2018-12-31T15:20:45.792Z] 15:20:45     INFO - GECKO(2775) | #11: nsDocShell::LoadURI(nsDocShellLoadState*) [docshell/base/nsDocShell.cpp:748]
[task 2018-12-31T15:20:45.793Z] 15:20:45     INFO -

Comment 31

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0da9808bcbd280f5534323ead433b560044db11

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f9080815cc8
Change WindowProxy type. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/18f95c6c1eb3
Change WindowProxyHolder's native type to BrowsingContext. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/85ed8082eac9
Split up PostMessage into two parts, one gathering information from the source and one gathering information from the target. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/fd540a8e08d4
Add code generation for array of cross origin properties. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/01b1087a6a25
Expose IsCrossOriginWhitelistedProp/AppendCrossOriginWhitelistedPropNames to DOM code. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/d66e7fe314e9
Add cross-process proxies for WindowProxy. r=bzbarsky

Comment 33

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=657b7e659035a944bb3e5ec3b3cfed8a3b0d1452

Comment 34

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4f9080815cc8
https://hg.mozilla.org/mozilla-central/rev/18f95c6c1eb3
https://hg.mozilla.org/mozilla-central/rev/85ed8082eac9
https://hg.mozilla.org/mozilla-central/rev/fd540a8e08d4
https://hg.mozilla.org/mozilla-central/rev/01b1087a6a25
https://hg.mozilla.org/mozilla-central/rev/d66e7fe314e9

Comment 35

[Jorg K (CEST = GMT+2)]

You've busted our Thunderbird builds. Something internal to M-C doesn't compile:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d024723d87afef943b5c8db2f6cd926c236ddc91

16:27.87 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/Alignment.h(44,5):  error: field has incomplete type 'mozilla::dom::WindowProxyHolder'
16:27.95   T mT;
16:28.01     ^
16:28.08 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/Maybe.h(151,3):  note: in instantiation of template class 'mozilla::detail::AlignasHelper<mozilla::dom::WindowProxyHolder>' requested here
16:28.15   MOZ_ALIGNAS_IN_STRUCT(T) unsigned char mStorage[sizeof(T)];
16:28.19   ^
16:28.19 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/Alignment.h(58,42):  note: expanded from macro 'MOZ_ALIGNAS_IN_STRUCT'
16:28.19 #define MOZ_ALIGNAS_IN_STRUCT(T) alignas(mozilla::detail::AlignasHelper<T>)

and more.

Maybe a unified compile issue.

Comment 36

[Jorg K (CEST = GMT+2)]

Attachment: 1353867-follow-up.patch

I think this is missing. If you approve, can you please get this landed on M-C directly. 1:20 AM now, I'll leave it in your hands.

Comment 37

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9034066 [details] [diff] [review]
1353867-follow-up.patch

Review of attachment 9034066 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable to me

Comment 38

[Jorg K (CEST = GMT+2)]

Yep, TB compiles with this.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/c9a17c8a3c3f
Follow-up: add missing include WindowProxyHolder.h to EventTarget.h. r=nika a=npotb

Comment 40

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 41

[Boris Zbarsky [:bzbarsky]]

Wait, hold on.  Why did you leave the forward-declaration?

And this is SO not npotb; you're changing a core Gecko header.  So the commit message doesn't make sense to me either...

Comment 42

[Jorg K (CEST = GMT+2)]

Hi Boris, you're right, after landing this it occurred to me that |class WindowProxyHolder;| should have been removed. But at nearly 2 AM, mistakes happen :-(

However, there is also
#include "mozilla/dom/Nullable.h"
and
struct Nullable;

which appears superfluous.

Somehow the bug is set to "leave-open", so is there more action to come here?

The sheriffs made up the "npotb", see IRC:
01:38:58 - jorgk: bogdan_tara|sheriffduty: And please make it r=nika
01:39:12 - bogdan_tara|sheriffduty: r=nika
01:39:18 - bogdan_tara|sheriffduty: and a= who? :)
01:39:50 - jorgk: find someone ;-)
01:41:22 - kwierso|afk: "a=thunderbird-only" likely works
01:41:49 - kwierso|afk: bogdan_tara|sheriffduty: ^
01:42:14 - kwierso|afk: or "a=npotb", since it doesn't affect firefox's build
01:42:19 - bogdan_tara|sheriffduty: could work, but a person would also work :)
01:44:35 - jorgk: it does affect FF's build <======
01:45:03 - bogdan_tara|sheriffduty: pushed
01:45:07 - jorgk: thanks

Let me know if you want to to submit another patch or whether I can leave the clean-up to you.

Comment 43

[Jorg K (CEST = GMT+2)]

Attachment: 1353867-follow-up2.patch

Looks like I was wrong about the |struct Nullable;|. Here's a follow-up.

Comment 44

[Peter Van der Beken [:peterv]]

The patch doesn't make sense to me. The compile error is in nsGlobalWindowInner.cpp, so that's where the include should have been added. Why was it added to EventTarget.h?

Comment 45

[Peter Van der Beken [:peterv]]

Comment on attachment 9034121 [details] [diff] [review]
1353867-follow-up2.patch

See comment 44, why does EventTarget.h needs changes? The build breakage was in nsGlobalWindowInner.cpp, so why isn't adding the include there instead of in EventTarget.h working?

Comment 46

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d96da52a97139675ef5ba44502fa9075f3f6b51e

Comment 47

[Jorg K (CEST = GMT+2)]

Attachment: 1353867-follow-up-2.patch - Peter's suggestion: Does not compile

I saw a felt kilometre of (very wordy confusing clang-cl) compile errors at about 1:30 AM, so what I got landed worked. The follow-up you disapproved also worked.

Your suggestion doesn't, see next attachment.

Peter, can you please fix the issue. It appears that your patches only compile by luck in the "unified source" environment used for FF. If a Thunderbird compile hadn't caught the issue, some other non-Firefox project would have found it a little later.

Comment 48

[Jorg K (CEST = GMT+2)]

Attachment: compile errors.txt

Really hard to tell what has gone wrong here, and even harder to tell for someone unfamiliar with the code how you want the forward-references structured.

Comment 49

[Boris Zbarsky [:bzbarsky]]

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

Comment 50

[Boris Zbarsky [:bzbarsky]]

And to be clear, Peter is on vacation this week, so asking him to fix non-emergency things (which includes Thunderbird bustage, honestly), especially as rudely as above, is a bit odd...

Comment 51

[Jorg K (CEST = GMT+2)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #49)
> 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...
Right. Looking at it.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #50)
> And to be clear, Peter is on vacation this week, so asking him to fix
> non-emergency things (which includes Thunderbird bustage, honestly),
> especially as rudely as above, is a bit odd...
I don't see the rude part. This is *not* TB bustage, any project compiling this code in a slightly different way will be busted. If I had the time I could find example bugs where missing includes that went unnoticed in FF made many projects fail to compile. If Peter is on vacation, surely one incorrectly placed include can wait a little. That said, I'll see where it's missing. Also, what's the answer to someone being on vacation and leaving bustage and crashes (bug 1517415) behind?

Comment 52

[Jorg K (CEST = GMT+2)]

Attachment: 1353867-follow-up-2.patch

This does it.

Comment 53

[Boris Zbarsky [:bzbarsky]]

> This is *not* TB bustage, any project compiling this code in a slightly different way will be busted.

Sure, it's still not an emergency.  Needs to be fixed, yes.  Right this second, no.

> If Peter is on vacation, surely one incorrectly placed include can wait a little. 

Sure.  No one said otherwise.

> Also, what's the answer to someone being on vacation and leaving bustage and crashes (bug 1517415) behind?

One of:

(a) Wait for them to get back.
(b) Get someone else to fix things.
(c) Back out.

depending on severity of the issues, availability of other people, expected duration of vacation, etc.  In this case, the right answer is (a).

Comment 54

[Neha Kochar [:neha]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #53)
> > This is *not* TB bustage, any project compiling this code in a slightly different way will be busted.
> 
> Sure, it's still not an emergency.  Needs to be fixed, yes.  Right this
> second, no.
> 
> > If Peter is on vacation, surely one incorrectly placed include can wait a little. 
> 
> Sure.  No one said otherwise.
> 
> > Also, what's the answer to someone being on vacation and leaving bustage and crashes (bug 1517415) behind?
> 
> One of:
> 
> (a) Wait for them to get back.
> (b) Get someone else to fix things.
> (c) Back out.
> 
> depending on severity of the issues, availability of other people, expected
> duration of vacation, etc.  In this case, the right answer is (a).

I agree. Peter will be back from vacation on Monday and can help you, Jorg, with the correct fix.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9034206 [details] [diff] [review]
1353867-follow-up-2.patch

>+++ b/dom/html/HTMLObjectElement.h

The include here should be in the .cpp, not .h, afaict.

dom/xul/XULFrameElement.cpp needs an include too.

Apart from that, looks good.

Comment 56

[Jorg K (CEST = GMT+2)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #53)
> > This is *not* TB bustage, any project compiling this code in a slightly different way will be busted.
> Sure, it's still not an emergency.  Needs to be fixed, yes.  Right this
> second, no.
All I can say is that we have 8 staff (not all developers), soon 12 and many volunteers. You want all those people sitting there twiddling thumbs, Dailies and try builds to fail? TB is not a toy-project any more where days/weeks or months of bustage can be tolerated. As I said, and you might think it's rude: The patches were wrong since the source only compiled under certain circumstances. And IMHO that's not OK, even if FF compiled. OK, I spent the time to find another example: bug 1345771 comment #15 where some BSD people started to complain.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #55)
> >+++ b/dom/html/HTMLObjectElement.h
> The include here should be in the .cpp, not .h, afaict.
You're sure? Further down in HTMLObjectElement.h we have
  Nullable<WindowProxyHolder> GetContentWindow(nsIPrincipal& aSubjectPrincipal);
and there's not other forward-declaration either in the file.

> dom/xul/XULFrameElement.cpp needs an include too.
Added. Compiler didn't complain, hmm.

Comment 57

[Jorg K (CEST = GMT+2)]

Attachment: 1353867-follow-up-2.patch

Comment 58

[Boris Zbarsky [:bzbarsky]]

> You want all those people sitting there twiddling thumbs

Look, you know I want Thunderbird to succeed.  But at the same time, Thunderbird bustage is _not_ an emergency for m-c, last I checked.  We will take fixes.  I personally will help with said fixes.  None of that changes the "not an emergency" status.

If that's not acceptable to Thunderbird, then you may need a way to land patches on your copy of m-c to get you green while you wait for upstream (m-c) to have the issue fixed.  That's how these things are normally done when you have an upstream that might cause you problems.

> The patches were wrong since the source only compiled under certain circumstances.

Sure.  No one is arguing about that.  Again, all I'm saying is that:

1) This wrongness is not an emergency from the point of view of m-c, though of course it should be fixed expeditiously, since it's causing problems for Thunderbird and has the potential to cause problems for other m-c consumers _including_ Firefox.

2) Talking about problems with _code_, not _people_, is a lot more productive and a lot less likely to make people upset.  Consider comment 35 with that in mind.  At least for me, "You've busted" implies intent/malice, and while I'm sure that's not what you meant, that's how it's coming across...

> and there's not other forward-declaration either in the file.

HTMLObjectElement.h includes (indirectly) EventTarget.h, which has the forward-declaration.

Comment 59

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9034212 [details] [diff] [review]
1353867-follow-up-2.patch

The HTMLObjectElement bit is still wrong.

By the way, it's really a better idea to put followup fixes like this in a new bug blocking this one...

Comment 60

[Jorg K (CEST = GMT+2)]

Comment on attachment 9034212 [details] [diff] [review]
1353867-follow-up-2.patch

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #59)
> The HTMLObjectElement bit is still wrong.
Well, adding WindowProxyHolder.h to HTMLObjectElement.cpp instead of HTMLObjectElement.h does not compile. There is also a forward-reference to WindowProxyHolder in the .h already (which I've removed in the patch).

Please reconsider.

> By the way, it's really a better idea to put followup fixes like this in a
> new bug blocking this one...
Hmm, well, the follow-up has landed here, so I think the fix of the follow-up can land here too, especially given that the bug is "leave-open" and Peter still seems to be working on it, see comment #46.

Comment 61

[Jorg K (CEST = GMT+2)]

Attachment: compile errors 2.txt

Very confusing errors with:

+++ b/dom/html/HTMLObjectElement.cpp
 #include "mozilla/dom/ElementInlines.h"
+#include "mozilla/dom/WindowProxyHolder.h"
 #include "nsAttrValueInlines.h"

+++ b/dom/html/HTMLObjectElement.h
 #include "mozilla/Attributes.h"
-#include "mozilla/dom/WindowProxyHolder.h"
 #include "nsGenericHTMLElement.h"
 #include "nsObjectLoadingContent.h"
 #include "nsIConstraintValidation.h"

 namespace mozilla {
 namespace dom {

 class HTMLFormSubmission;
 template <typename T>
 struct Nullable;
+class WindowProxyHolder; <== restoring original.

Am I missing something?

Comment 62

[Boris Zbarsky [:bzbarsky]]

Boy, C++ error messages.  ;)

What you're seeing there is that BindingUtils.cpp calls ToJSValue on a WindowProxyHolder.  That requires knowing what concrete thing WindowProxyHolder is, but BindingUtils.cpp is not including WindowProxyHolder.h.  It should.

It _does_ include HTMLObjectElement.h because of some <object> special-casing in there, which is why including WindowProxyHolder.h in there helped.

Comment 63

[Peter Van der Beken [:peterv]]

BTW, I was working on fixing this myself, even though I'm on PTO. I am also looking at the crashes (as evidenced by my comments here and in bug 1517415). So I don't think comments about me leaving bustage and crashes behind are justified. Let me know if my help is still needed here.

Comment 64

[Jorg K (CEST = GMT+2)]

Attachment: 1353867-follow-up-2.patch

Boris, that's for digging into this.

This works. Allow my some personal comments.

Comment #25:
  "You've busted our Thunderbird builds. Something internal to M-C doesn't compile:"
That's a fact. I could have said, the code you landed busted ...
Why does that imply intent/malice? I don't get it.

Comment #63:
I knew nothing about PTO until Boris pointed it out. The question of how Mozilla address these problems in general was sufficiently answered: Wait, fix it yourself (doing that here) or backout (not warranted, since TB is not Tier-1). Looks like my general comment was given a personal interpretation.

Why don't we focus on the facts?

Comment 65

[Jorg K (CEST = GMT+2)]

The checkin-needed is for 1353867-follow-up-2.patch.

Comment 66

[Razvan Maries]

Error while trying to land:

applying 1353867-follow-up-2.patch
patching file dom/html/HTMLObjectElement.cpp
Hunk #1 FAILED at 3
1 out of 1 hunks FAILED -- saving rejects to file dom/html/HTMLObjectElement.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1353867-follow-up-2.patch

Comment 67

[Jorg K (CEST = GMT+2)]

Attachment: 1353867-follow-up-2.patch

Rebased for the nsIDocoment.h to mozilla/dom/Document.h change from
Bug 1517241 - Rename nsIDocument to mozilla::dom::Document

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f612a041c69c
Follow-up, take 2: revert rev c9a17c8a3c3f and add include of WindowProxyHolder.h where it was missing. r=bz

Comment 69

[Noemi Erli[:noemi_erli]]

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

Comment 70

[Peter Van der Beken [:peterv]]

I'm going to move the Location patch to a separate bug. WindowProxy has already landed (with some bumps).

Comment 71

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed5e85377b8bbc5efdc3c9f367c0b5c0b8dbcd18