1354249 - Print preview tab should be in same TabGroup as original document

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Local linux64 debug build from m-c cset facaf90aeaaf.

STR:
1. Go to http://conorlastowka.com/book/CitationNeededBook-Sample.pdf
2. Select the print icon from the hamburger menu

Expected:
- opens a print preview pane

Actual:
- content process crashes with assertion at [1]. This leaves a "preparing" dialog on top of the crashed-tab message.

[1] Go to http://conorlastowka.com/book/CitationNeededBook-Sample.pdf

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

The problem here is that we're doing print preview in a different tab group than the original document. Normally that works fine because the two documents shouldn't need to interact. However, pdf.js documents use "mozPrintCallback", which runs code from the original tab in the print preview tab (yuck).

I talked to Olli and he thinks we should put the print preview tab in the same TabGroup as the original. I decided to repurpose the sameProcessAsFrameLoader attribute from nsIBrowser so that we also use the same TabGroup. The only consumers of this attribute are print preview and view source, which seems fine.

Michael, let me know if you'd rather not review this. I can wait for Olli to open his queue.

Comment 2

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

Comment on attachment 8855573 [details] [diff] [review]
patch

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

Patch looks good. I have some style nits, and am seeing an opportunity to merge this code path with the one which we're currently using to open windows with openers within the current process, which could help clean up the TabGroup logic, so I'm going to r- until we've considered it.

::: dom/base/nsFrameLoader.cpp
@@ +2835,5 @@
>  static
> +void
> +GetContentParent(Element* aBrowser,
> +                 ContentParent** aSameProcessAs,
> +                 TabParent** aSameTabGroupAs)

Usually in our codebase when you see methods which take a T**, we assume that the caller has to getter_AddRefs. In this code we don't transfer any ownership for these outparams, which makes me uncomfortable that someone will mess it up. Can you at least add a comment explaining that this function does _not_ transfer ownership?

::: dom/ipc/ContentChild.cpp
@@ +777,1 @@
>                                             newTabContext, aChromeFlags);

If I remember correctly, in this code path the current way we propagate the correct TabGroup to the new window in this TabChild is through the nsIEventTarget which we set on the TabChild a few lines down. In TabGroup::GetFromWindowActor, we pull this nsIEventTarget down and try to recover the TabGroup from it.

IIRC as well, this is the only code path which uses this nsIEventTarget workaround. If you make the change to the TabChild's constructor parameter which I mention in the comment below, you should just pass the TabGroup* down here, and remove the nsIEventTarget workaround from the TabChild code.

@@ +1492,5 @@
>      NS_IdleDispatchToCurrentThread(firstIdleTask.forget());
>    }
>  
> +  return nsIContentChild::RecvPBrowserConstructor(aActor, aTabId, aSameTabGroupAs,
> +                                                  aContext,

nit: the wrapping here isn't very nice

::: dom/ipc/PContent.ipdl
@@ +319,5 @@
>      // type PopupIPCTabContext.  The parent checks that if the opener is a
>      // browser element, the context is also for a browser element.
>      //
>      // Keep the last 3 attributes in sync with GetProcessAttributes!
> +    async PBrowser(TabId tabId, TabId sameTabGroupAs,

Can you add a comment to this method noting that sameTabGroupAs is ignored when the SendPBrowserConstructor is called on ContentChild? This parameter is only meaningful when being passed into a child process, not out of it.

It would also be nice if we could assert that sameTabGroupAs is 0 in the parent process, just for clarity.

::: dom/ipc/TabChild.cpp
@@ +366,5 @@
>  }
>  
>  TabChild::TabChild(nsIContentChild* aManager,
>                     const TabId& aTabId,
> +                   const TabId& aSameTabGroupAs,

I think I would prefer it if instead of passing aSameTabGroupAs here we passed a TabGroup* directly. TabChild::Create() could then call FindTabChild(aSameTabGroupAs)->TabGroup() before passing it into the constructor.

If we did that I would change mSameTabGroupAs to be a RefPtr<TabGroup>, return that from TabChild::TabGroup instead of going to the nsPIDOMWindowOuter, and then remove the nsIEventTarget TabGroup workaround which is currently used for when opening windows within the same process. It seems like it would clean up one of the uglier corners of the TabGroup transfer story.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v2

I think this is pretty close to what you asked for. There are a couple differences though:

Your explanation for why we need the GetFromWindowActor code is not quite right. Suppose that the parent process sends a PBrowserConstructor message to the child. On the I/O thread, we need to figure out an event target for that message immediately so that we can dispatch it. GetConstructedEventTarget does that by creating a new TabGroup and getting the event target from it. At this point, though, the TabChild object doesn't exist yet.

When we do create the TabChild (and an nsGlobalWindow for it), we need to find the TabGroup that we created earlier on the I/O thread. The only place where that is stored is in the IPC event target map. So we have to look up the event target and then get to a TabGroup from there. There isn't any sort of easy refactoring that can avoid doing this.

However, I tried to follow most of your suggestions about how to pass around the TabGroup. I think they do make the code cleaner. One thing to note is that we can't find the TabGroup from the event target in AllocPBrowserChild because the TabChild hasn't been assigned an actor ID yet. So that's why I did it in RecvPBrowserConstructor (which runs just a little later).

I also had to add the IgnoreSentinel thing because I forgot to handle sentinels in ContentChild::GetConstructedEventTarget.

Comment 4

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

Comment on attachment 8857655 [details] [diff] [review]
patch v2

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

Looks great :)

::: dom/base/nsFrameLoader.cpp
@@ +2832,5 @@
>    return NS_OK;
>  }
>  
>  static
> +Tuple<ContentParent*, TabParent*>

ooh, fancy.

::: dom/ipc/TabChild.cpp
@@ +571,5 @@
>  TabChild::Init()
>  {
> +  if (!mTabGroup) {
> +    mTabGroup = TabGroup::GetFromActor(this);
> +  }

If mTabGroup is already set, it might be worthwhile to `MOZ_ASSERT_IF(mTabGroup, !TabGroup::GetFromActor(this));` as a check on the logic in GetConstructedEventTarget.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2268ecf05d
Use same TabGroup as original tab for print preview (r=mystor)

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Filed bug 1356090 to add a test for this. I'm not sure how we normally test pdf.js.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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