Blake Kaplan (:mrbkap) (inactive) Assignee
	Description • 8 years ago

Per bug 1165309 comment 45, we need to fix view source in a new window.

While investigating this bug, I actually realized that my patch in bug 1165309 isn't actually working as I intended. More details in a hacky patch to come.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 1 • 8 years ago

This patch also makes sure that we correctly grab a weak reference to the related window instead of just setting a "relatedBrowser" property directly on the JS object (which shadows the XBL property getter).

It's pretty ugly. Mike, what do you think?

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 3 • 8 years ago

Comment on attachment 8783120 [details] [diff] [review]
Patch v1

Sorry for the bugspam. I just got mozreview to work for me.

	Mike Conley (:mconley) (:⚙️)
	Comment 4 • 8 years ago
mozreview-review

Comment on attachment 8783122 [details]
Bug 1296785 - Make view-source in a new window work correctly

https://reviewboard.mozilla.org/r/73062/#review70876

::: toolkit/components/viewsource/content/viewSource.js:683
(Diff revision 1)
> +    // XX Removing and re-adding the browser from and to the DOM strips its
> +    // XBL properties. Save and restore relatedBrowser. Note that when we
> +    // restore relatedBrowser, there won't yet be a binding or setter. This
> +    // works in conjunction with the hack in <xul:browser>'s constructor to
> +    // re-get the weak reference to it.
> +    let relatedBrowser = this.browser.relatedBrowser;

Hrm. We might have to do similar hackery in tabbrowser's updateBrowserRemoteness to be consistent - though I can't really think of a scenario where we'd need it off the top of my head.

::: toolkit/content/widgets/browser.xml:903
(Diff revision 1)
> +          // XXX tabbrowser.js sets "relatedBrowser" as a direct property on
> +          // some browsers before they are put into a DOM (and get a binding).
> +          // This hack makes sure that we hold a weak reference to the other
> +          // browser (and go through the proper getter and setter).

Woof. :(

Good catch - this was likely biting us, so we were probably leaking browsers already.

Thanks for documenting it.

XBL, you've done it again.

![Sobbing](http://i.giphy.com/D3ggX9iWqOHza.gif)

	Mike Conley (:mconley) (:⚙️)
	Comment 5 • 8 years ago
mozreview-review

Comment on attachment 8783122 [details]
Bug 1296785 - Make view-source in a new window work correctly

https://reviewboard.mozilla.org/r/73062/#review70878

Whoops, missed an issue.

::: toolkit/content/widgets/browser.xml:903
(Diff revision 1)
> +          // XXX tabbrowser.js sets "relatedBrowser" as a direct property on
> +          // some browsers before they are put into a DOM (and get a binding).
> +          // This hack makes sure that we hold a weak reference to the other
> +          // browser (and go through the proper getter and setter).

tabbrowser.xml, not tabbrowser.js

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 6 • 8 years ago

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #4)
> Hrm. We might have to do similar hackery in tabbrowser's
> updateBrowserRemoteness to be consistent - though I can't really think of a
> scenario where we'd need it off the top of my head.

I don't actually think we do there because we don't add or remove the browser's DOM node from the document. Otherwise we would have to do it there.

	Pulsebot
	Comment 7 • 8 years ago

Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c94db70f3f15
Make view-source in a new window work correctly. r=mconley

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 8 years ago
bugherder

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

	Mike Conley (:mconley) (:⚙️)
	Comment 9 • 8 years ago

ni for comment 6

	Mike Conley (:mconley) (:⚙️)
	Comment 10 • 8 years ago

(In reply to Blake Kaplan (:mrbkap) from comment #6)

> I don't actually think we do there because we don't add or remove the
> browser's DOM node from the document. Otherwise we would have to do it there.

Are you sure?: http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/browser/base/content/tabbrowser.xml#1620

Or am I missing something?

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 11 • 8 years ago

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #10)
> Or am I missing something?

I am blind. Of course you're right. Bug and patch coming up.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 12 • 8 years ago

Bug 1299334.