Closed Bug 1296785 Opened 4 years ago Closed 4 years ago

[e10s] [dom.ipc.processCount>1] Fix view-source for a new window

Categories

(Toolkit :: View Source, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
e10s ? ---
firefox51 --- fixed

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

(Keywords: regression, Whiteboard: [e10s-multi:M1])

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
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?
Attachment #8783120 - Flags: review?(mconley)
Attachment #8783122 - Flags: review?(mconley)
Comment on attachment 8783120 [details] [diff] [review]
Patch v1

Sorry for the bugspam. I just got mozreview to work for me.
Attachment #8783120 - Attachment is obsolete: true
Attachment #8783120 - Flags: review?(mconley)
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)
Attachment #8783122 - Flags: review?(mconley) → 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
tracking-e10s: --- → ?
(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.
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
https://hg.mozilla.org/mozilla-central/rev/c94db70f3f15
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
ni for comment 6
Flags: needinfo?(mconley)
(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?
Flags: needinfo?(mconley) → needinfo?(mrbkap)
(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.
Flags: needinfo?(mrbkap)
Depends on: 1299334
You need to log in before you can comment on or make changes to this bug.