about:tabcrashed uses events bubbled up to browser.js in order to message things to the chrome-level. We should also move the tracking of how many crashed browsers there are from SessionStore to something like TabCrashReporter. We should probably use RemotePageManager instead. Because about:tabcrashed is actually loaded internally by the docShell, this will also require some slight modification to RemotePageManager.

Bug 1220929 - RemotePageManager should use documentURI and allow special URLs with query params. r?Mossop

Bug 1220929 - RemotePageManager addMessageListener should be able to take listener objects. r?Mossop

Bug 1220929 - RemotePageManager should use waiveXrays on listener objects. r?Mossop.

Bug 1220929 - RemotePageManager should let us get port for a browser. r?Mossop

Bug 1220929 - Refactor aboutTabCrashed to use RemotePageManager. r?felipe.

Bug 1220929 - Rename TabCrashReporter to TabCrashHandler to reflect its actual purpose. r?felipe

Bug 1220929 - Remove crashed tab / browser count from SessionStore. r?Mossop

Comment on attachment 8682287 [details] MozReview Request: Bug 1220929 - RemotePageManager should use documentURI and allow special URLs with query params. r=Mossop https://reviewboard.mozilla.org/r/24027/#review21547 ::: toolkit/modules/RemotePageManager.jsm:21 (Diff revision 1) > +]; Special casing this seems unfortunate. Lots of people keep talking about supporting regexps for the urls, can we just do that? "JUST"!

Comment on attachment 8682288 [details] MozReview Request: Bug 1220929 - Add test for RemotePage's portsForBrowser. r=Mossop https://reviewboard.mozilla.org/r/24029/#review21549 ::: toolkit/modules/RemotePageManager.jsm:46 (Diff revision 1) > + throw "Listener object should implement receiveMessage"; This error isn't going to let a developer helpfully understand which listener was at fault. Move the check to addListener instead.

Comment on attachment 8682289 [details] MozReview Request: Bug 1220929 - RemotePageManager should use waiveXrays on listener objects. r?bholley. https://reviewboard.mozilla.org/r/24031/#review21551 ::: toolkit/modules/RemotePageManager.jsm:45 (Diff revision 1) > - if (!("receiveMessage" in listener)) { > + let waived = Cu.waiveXrays(listener); Can you add a comment about why this is necessary, I can't guess and so future maintainers proibably won't know either.

Comment on attachment 8682290 [details] MozReview Request: Bug 1220929 - RemotePageManager should let us get all ports for a browser. r=Mossop https://reviewboard.mozilla.org/r/24033/#review21553 ::: toolkit/modules/RemotePageManager.jsm:174 (Diff revision 1) > + portForBrowser: function(browser) { It's possible that there can be multiple ports for a browser if the interesting url is an inner frame. So this should probably return an array of ports. Callers can then assume there is only ever 1.

https://reviewboard.mozilla.org/r/24027/#review21547 > Special casing this seems unfortunate. Lots of people keep talking about supporting regexps for the urls, can we just do that? > > "JUST"! I spent some time looking at this, and I think it'd be good follow-up work, but maybe out of scope for this bug.

https://reviewboard.mozilla.org/r/24031/#review21551 > Can you add a comment about why this is necessary, I can't guess and so future maintainers proibably won't know either. Done, but I'm not 100% confident I've done the right thing, security-wise. I'll r? bholley.

Comment on attachment 8682287 [details] MozReview Request: Bug 1220929 - RemotePageManager should use documentURI and allow special URLs with query params. r=Mossop https://reviewboard.mozilla.org/r/24027/#review21569 Please file a follow-up bug

Comment on attachment 8682293 [details] MozReview Request: Bug 1220929 - Remove crashed tab / browser count from SessionStore. r=Mossop https://reviewboard.mozilla.org/r/24049/#review21571

Comment on attachment 8682288 [details] MozReview Request: Bug 1220929 - Add test for RemotePage's portsForBrowser. r=Mossop Bug 1220929 - RemotePageManager addMessageListener should be able to take listener objects. r?Mossop,bholley

Comment on attachment 8682289 [details] MozReview Request: Bug 1220929 - RemotePageManager should use waiveXrays on listener objects. r?bholley. Bug 1220929 - RemotePageManager should use waiveXrays on listener objects. r?bholley.

Comment on attachment 8682290 [details] MozReview Request: Bug 1220929 - RemotePageManager should let us get all ports for a browser. r=Mossop Bug 1220929 - RemotePageManager should let us get all ports for a browser. r?Mossop

Comment on attachment 8682291 [details] MozReview Request: Bug 1220929 - Refactor aboutTabCrashed to use RemotePageManager. r=felipe Bug 1220929 - Refactor aboutTabCrashed to use RemotePageManager. r?felipe.

Comment on attachment 8682292 [details] MozReview Request: Bug 1220929 - Rename TabCrashReporter to TabCrashHandler to reflect its actual purpose. r=felipe Bug 1220929 - Rename TabCrashReporter to TabCrashHandler to reflect its actual purpose. r?felipe

Comment on attachment 8682293 [details] MozReview Request: Bug 1220929 - Remove crashed tab / browser count from SessionStore. r=Mossop Bug 1220929 - Remove crashed tab / browser count from SessionStore. r?Mossop

Bug 1220929 - Harden browser_crashedTabs.js against races. r?felipe

Hey bholley - requesting review on attachment 8682289 [details] and attachment 8682288 [details] because I'm waiving Xray wrappers here, and I'm not 100% certain that's the right thing to do. Essentially, RemotePageManager allows us to expose sendAsyncMessage to less-privileged content if the URL of the page is registered in the chrome process. Up until now, the listeners that the less-privileged content registers have been callback functions. The patches I've requested review from you on make it so that we can take objects that have a receiveMessage function property as well. In order to see receiveMessage, however, I have to waive Xrays. Is that the right way to do this?

Comment on attachment 8682290 [details] MozReview Request: Bug 1220929 - RemotePageManager should let us get all ports for a browser. r=Mossop https://reviewboard.mozilla.org/r/24033/#review21709 Looks good, I'd love to see a quick test for this though

Comment on attachment 8682288 [details] MozReview Request: Bug 1220929 - Add test for RemotePage's portsForBrowser. r=Mossop https://reviewboard.mozilla.org/r/24029/#review21711 Looks like this might not be right, I'd also like to see a test here. ::: toolkit/modules/RemotePageManager.jsm:67 (Diff revision 2) > + this.listeners.set(name, new Set([listener])); Don't you need to store waived in the case of an object or callListeners won't be able to access receiveMessage.

Comment on attachment 8682288 [details] MozReview Request: Bug 1220929 - Add test for RemotePage's portsForBrowser. r=Mossop https://reviewboard.mozilla.org/r/24029/#review21713 ::: toolkit/modules/RemotePageManager.jsm:61 (Diff revision 2) > + // See: https://developer.mozilla.org/en-US/docs/Xray_vision#Xray_semantics_for_Object_and_Array > + let waived = Cu.waiveXrays(listener); > + if (!("receiveMessage" in waived)) { > + throw "Listener object should implement receiveMessage method"; > + } > + } > + this.listeners.set(name, new Set([listener])); > + } else { > + this.listeners.get(name).add(listener); > + } So this definitiely isn't the right thing - thanks for flagging me for review! The fact that addMessageListener is being called with arbitrary content objects suggests to me that we're poking a hole somewhere and not using WebIDL like we should be. Let's talk on IRC when you're free.

Comment on attachment 8682292 [details] MozReview Request: Bug 1220929 - Rename TabCrashReporter to TabCrashHandler to reflect its actual purpose. r=felipe https://reviewboard.mozilla.org/r/24047/#review21715

Comment on attachment 8682769 [details] MozReview Request: Bug 1220929 - Harden browser_crashedTabs.js against races. r=felipe https://reviewboard.mozilla.org/r/24153/#review21717

Comment on attachment 8682291 [details] MozReview Request: Bug 1220929 - Refactor aboutTabCrashed to use RemotePageManager. r=felipe https://reviewboard.mozilla.org/r/24035/#review21719

Comment on attachment 8682287 [details] MozReview Request: Bug 1220929 - RemotePageManager should use documentURI and allow special URLs with query params. r=Mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24027/diff/1-2/

Comment on attachment 8682290 [details] MozReview Request: Bug 1220929 - RemotePageManager should let us get all ports for a browser. r=Mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24033/diff/2-3/

Comment on attachment 8682288 [details] MozReview Request: Bug 1220929 - Add test for RemotePage's portsForBrowser. r=Mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24029/diff/2-3/

Comment on attachment 8682291 [details] MozReview Request: Bug 1220929 - Refactor aboutTabCrashed to use RemotePageManager. r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24035/diff/2-3/

Comment on attachment 8682292 [details] MozReview Request: Bug 1220929 - Rename TabCrashReporter to TabCrashHandler to reflect its actual purpose. r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24047/diff/2-3/

Comment on attachment 8682293 [details] MozReview Request: Bug 1220929 - Remove crashed tab / browser count from SessionStore. r=Mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24049/diff/2-3/

Comment on attachment 8682769 [details] MozReview Request: Bug 1220929 - Harden browser_crashedTabs.js against races. r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24153/diff/1-2/

https://reviewboard.mozilla.org/r/24029/#review21713 > So this definitiely isn't the right thing - thanks for flagging me for review! > > The fact that addMessageListener is being called with arbitrary content objects suggests to me that we're poking a hole somewhere and not using WebIDL like we should be. > > Let's talk on IRC when you're free. I've abandoned the idea of passing objects to RemotePageManager's addMessageListener, as per bholley's suggestion.

Comment on attachment 8682288 [details] MozReview Request: Bug 1220929 - Add test for RemotePage's portsForBrowser. r=Mossop https://reviewboard.mozilla.org/r/24029/#review22225

Comment on attachment 8682287 [details] MozReview Request: Bug 1220929 - RemotePageManager should use documentURI and allow special URLs with query params. r=Mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24027/diff/2-3/

Comment on attachment 8682290 [details] MozReview Request: Bug 1220929 - RemotePageManager should let us get all ports for a browser. r=Mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24033/diff/3-4/

Comment on attachment 8682288 [details] MozReview Request: Bug 1220929 - Add test for RemotePage's portsForBrowser. r=Mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24029/diff/3-4/

Comment on attachment 8682291 [details] MozReview Request: Bug 1220929 - Refactor aboutTabCrashed to use RemotePageManager. r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24035/diff/3-4/

Comment on attachment 8682292 [details] MozReview Request: Bug 1220929 - Rename TabCrashReporter to TabCrashHandler to reflect its actual purpose. r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24047/diff/3-4/

Comment on attachment 8682293 [details] MozReview Request: Bug 1220929 - Remove crashed tab / browser count from SessionStore. r=Mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24049/diff/3-4/

Comment on attachment 8682769 [details] MozReview Request: Bug 1220929 - Harden browser_crashedTabs.js against races. r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24153/diff/2-3/

https://hg.mozilla.org/integration/fx-team/rev/4f8419bc85b36dbc671abbb9d6420c5596efc43c Bug 1220929 - RemotePageManager should use documentURI and allow special URLs with query params. r=Mossop https://hg.mozilla.org/integration/fx-team/rev/f531919a3be6a3e7436d782f6e28893eed7b15aa Bug 1220929 - RemotePageManager should let us get all ports for a browser. r=Mossop https://hg.mozilla.org/integration/fx-team/rev/fa183fc905e2a9831cf7f765bb5fa63d36ee2744 Bug 1220929 - Add test for RemotePage's portsForBrowser. r=Mossop https://hg.mozilla.org/integration/fx-team/rev/e2b3c92f2953fada805db20e04dc90a20fdcec97 Bug 1220929 - Refactor aboutTabCrashed to use RemotePageManager. r=felipe https://hg.mozilla.org/integration/fx-team/rev/c496ae71f2b3e1231402bd99ca91b461c96e2e2e Bug 1220929 - Rename TabCrashReporter to TabCrashHandler to reflect its actual purpose. r=felipe https://hg.mozilla.org/integration/fx-team/rev/ed6036c148ba68e2d413a4e5c7f6ffc243e03155 Bug 1220929 - Remove crashed tab / browser count from SessionStore. r=Mossop https://hg.mozilla.org/integration/fx-team/rev/2465c9539de16832cc58305d17a053ac73df1730 Bug 1220929 - Harden browser_crashedTabs.js against races. r=felipe

https://hg.mozilla.org/mozilla-central/rev/4f8419bc85b3 https://hg.mozilla.org/mozilla-central/rev/f531919a3be6 https://hg.mozilla.org/mozilla-central/rev/fa183fc905e2 https://hg.mozilla.org/mozilla-central/rev/e2b3c92f2953 https://hg.mozilla.org/mozilla-central/rev/c496ae71f2b3 https://hg.mozilla.org/mozilla-central/rev/ed6036c148ba https://hg.mozilla.org/mozilla-central/rev/2465c9539de1