Implement Cross-Process async window proxies

RESOLVED FIXED in mozilla66

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: Nika, Assigned: peterv)

Tracking

(Depends on 1 bug, Blocks 3 bugs)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Fission Milestone:M1)

Details

Attachments

(8 attachments, 8 obsolete attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
833 bytes, patch
Nika
: review+
Details | Diff | Splinter Review
5.47 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
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.
(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 :)
Priority: -- → P3
(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.
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.
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).
Blocks: oop-frames
(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.
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 :-).
Depends on: browsingcontext
See Also: → 1436504
Reporter

Updated

7 months ago
Assignee: nobody → peterv
Assignee

Comment 16

6 months ago
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.
Assignee

Comment 17

6 months ago
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
Assignee

Updated

6 months ago
Blocks: 1510760
Attachment #9026950 - Attachment description: Bug 1353867 - Change WindowProxy type. → Bug 1353867 - Change WindowProxy type. r?bzbarsky!
Attachment #9026951 - Attachment description: Bug 1353867 - Change WindowProxyHolder's native type to BrowsingContext. → Bug 1353867 - Change WindowProxyHolder's native type to BrowsingContext. r?bzbarsky!
Attachment #9026952 - Attachment description: Bug 1353867 - Split up PostMessage into two parts, one gathering information from the source and one gathering information from the target. → Bug 1353867 - Split up PostMessage into two parts, one gathering information from the source and one gathering information from the target. r=bzbarsky
Attachment #9026954 - Attachment description: Bug 1353867 - Add code generation for array of cross origin properties. → Bug 1353867 - Add code generation for array of cross origin properties. r=bzbarsky
Attachment #9026955 - Attachment description: Bug 1353867 - Expose IsCrossOriginWhitelistedProp/AppendCrossOriginWhitelistedPropNames to DOM code. → Bug 1353867 - Expose IsCrossOriginWhitelistedProp/AppendCrossOriginWhitelistedPropNames to DOM code. r?bzbarsky!
Attachment #9026956 - Attachment description: Bug 1353867 - Add cross-process proxies for WindowProxy. → Bug 1353867 - Add cross-process proxies for WindowProxy. r?bzbarsky!
Attachment #9026957 - Attachment description: Bug 1353867 - Add cross-process proxies for Location. → Bug 1353867 - Add cross-process proxies for Location. r?bzbarsky!

Updated

5 months ago
Status: NEW → ASSIGNED
Attachment #9026950 - Attachment description: Bug 1353867 - Change WindowProxy type. r?bzbarsky! → Bug 1353867 - Change WindowProxy type. r=bzbarsky
Attachment #9026955 - Attachment description: Bug 1353867 - Expose IsCrossOriginWhitelistedProp/AppendCrossOriginWhitelistedPropNames to DOM code. r?bzbarsky! → Bug 1353867 - Expose IsCrossOriginWhitelistedProp/AppendCrossOriginWhitelistedPropNames to DOM code. r=bzbarsky
Attachment #9026951 - Attachment description: Bug 1353867 - Change WindowProxyHolder's native type to BrowsingContext. r?bzbarsky! → Bug 1353867 - Change WindowProxyHolder's native type to BrowsingContext. r=bzbarsky
Assignee

Updated

5 months ago
Blocks: 1516343
Attachment #9026956 - Attachment description: Bug 1353867 - Add cross-process proxies for WindowProxy. r?bzbarsky! → Bug 1353867 - Add cross-process proxies for WindowProxy. r=bzbarsky

Comment 27

5 months ago
Pushed by pvanderbeken@mozilla.com:
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
Assignee

Comment 28

5 months ago
The location patch still needs final review and landing.
Keywords: leave-open
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 -
Flags: needinfo?(peterv)

Comment 32

5 months ago
Pushed by pvanderbeken@mozilla.com:
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 35

5 months ago
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.
Flags: needinfo?(peterv)

Updated

5 months ago
Flags: needinfo?(peterv)
Flags: needinfo?(bzbarsky)

Comment 36

5 months ago
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.
Attachment #9034066 - Flags: review?(peterv)
Attachment #9034066 - Flags: review?(bzbarsky)
Comment on attachment 9034066 [details] [diff] [review]
1353867-follow-up.patch

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

Seems reasonable to me
Attachment #9034066 - Flags: review?(peterv)
Attachment #9034066 - Flags: review?(bzbarsky)
Attachment #9034066 - Flags: review+

Comment 38

5 months ago
Yep, TB compiles with this.
Flags: needinfo?(peterv)
Flags: needinfo?(bzbarsky)

Comment 39

5 months ago
Pushed by btara@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/c9a17c8a3c3f
Follow-up: add missing include WindowProxyHolder.h to EventTarget.h. r=nika a=npotb
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...
Flags: needinfo?(jorgk)

Comment 42

5 months ago
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.
Flags: needinfo?(jorgk)

Comment 43

5 months ago
Posted patch 1353867-follow-up2.patch (obsolete) — Splinter Review
Looks like I was wrong about the |struct Nullable;|. Here's a follow-up.
Attachment #9034121 - Flags: review?(bzbarsky)
Assignee

Comment 44

5 months ago
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?
Flags: needinfo?(jorgk)
Attachment #9034121 - Flags: review?(bzbarsky) → review?(peterv)
Assignee

Comment 45

5 months ago
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?
Attachment #9034121 - Flags: review?(peterv) → review-

Comment 47

5 months ago
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.
Flags: needinfo?(jorgk)

Comment 48

5 months ago
Posted file compile errors.txt (obsolete) —
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.
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...
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

5 months ago
(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

5 months ago
Posted patch 1353867-follow-up-2.patch (obsolete) — Splinter Review
This does it.
Attachment #9034121 - Attachment is obsolete: true
Attachment #9034199 - Attachment is obsolete: true
Attachment #9034201 - Attachment is obsolete: true
Attachment #9034206 - Flags: review?(peterv)
Attachment #9034206 - Flags: review?(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).
Depends on: 1517467
(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.
Depends on: 1517518
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.
Attachment #9034206 - Flags: review?(peterv)
Attachment #9034206 - Flags: review?(bzbarsky)
Attachment #9034206 - Flags: review-

Comment 56

5 months ago
(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

5 months ago
Posted patch 1353867-follow-up-2.patch (obsolete) — Splinter Review
Attachment #9034206 - Attachment is obsolete: true
Attachment #9034212 - Flags: review?(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 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...
Attachment #9034212 - Flags: review?(bzbarsky) → review-

Comment 60

5 months ago
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.
Attachment #9034212 - Flags: review- → review?(bzbarsky)

Comment 61

5 months ago
Posted file compile errors 2.txt (obsolete) —
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?
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.
Assignee

Comment 63

5 months ago
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

5 months ago
Posted patch 1353867-follow-up-2.patch (obsolete) — Splinter Review
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?
Attachment #9034212 - Attachment is obsolete: true
Attachment #9034221 - Attachment is obsolete: true
Attachment #9034212 - Flags: review?(bzbarsky)
Attachment #9034273 - Flags: review?(bzbarsky)
Attachment #9034273 - Flags: review?(bzbarsky) → review+

Updated

5 months ago
Keywords: checkin-needed

Comment 65

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

Comment 66

5 months ago
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
Flags: needinfo?(peterv)
Keywords: checkin-needed

Comment 67

5 months ago
Rebased for the nsIDocoment.h to mozilla/dom/Document.h change from
Bug 1517241 - Rename nsIDocument to mozilla::dom::Document
Attachment #9034273 - Attachment is obsolete: true
Flags: needinfo?(peterv)
Attachment #9034363 - Flags: review+

Updated

5 months ago
Keywords: checkin-needed

Comment 68

5 months ago
Pushed by csabou@mozilla.com:
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
Keywords: checkin-needed
Assignee

Updated

4 months ago
Blocks: 1518202
Assignee

Comment 70

4 months ago

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

Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Keywords: leave-open
Resolution: --- → FIXED
Attachment #9026957 - Attachment is obsolete: true

Updated

4 months ago
Target Milestone: --- → mozilla66
Assignee

Updated

4 months ago
Blocks: 1518787
Assignee

Updated

4 months ago
Blocks: 1517316

Updated

4 months ago
Fission Milestone: --- → M1
Component: DOM → DOM: Core & HTML
Product: Core → Core
Reporter

Updated

2 months ago
Blocks: 1541728
You need to log in before you can comment on or make changes to this bug.