Closed Bug 449734 Opened 11 years ago Closed 9 years ago

Preserve plugin state when dragging a tab between browser windows

Categories

(Core :: Document Navigation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: sylvain.pasche, Assigned: mats)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: parity-chrome)

Attachments

(7 files, 5 obsolete files)

If you open a flash video on a tab and drag it to another window, the video is reset. It would be nice if the plugin state was kept when dragging a tab (Opera does this on Linux).

(by the way, dragging a <video> does not reset its state)
Unfortunately, plugins use native widgets, and those can't be reparented on some OSes (Windows?  I don't recall), so we can't do this in general.  We might be able to special-case things here on an OS-by-OS basis, but it'll take some work, icky ifdefs, and we have bigger fish to fry first (like preserving all the non-plugin layout, say).

This certainly won't happen until after view/widget elimination is done.
Depends on: 337801, widget-removal
widgets can be reparented on Linux and Windows. Dunno if plugins can be reparented on Mac.
Flags: wanted1.9.1?
Duplicate of this bug: 466189
(In reply to comment #2)
> widgets can be reparented on Linux and Windows. Dunno if plugins can be
> reparented on Mac.

Safari manages to maintain at least youtube state when dragging to a new tab, so I suspect so.
Also, Chrome and Safari both maintain youtube on windows.
(In reply to comment #2)
> widgets can be reparented on Linux and Windows. Dunno if plugins can be
> reparented on Mac.

Plugins don't really have widgets on Mac so there isn't a problem.
The key thing here is that in the new plugin world we have to make sure that the plugin state is owned by the element, not the frame. That includes its native widget, if there is one. When we create a frame for the plugin (nsPluginFrame, say), that native widget would be reparented to the correct place. When there's no frame the widget will have to be parented to a hidden window or the toplevel window for the document (and hidden), or something like that. Although if we get to compositor first, then we can just have the widget always belong to the toplevel window since there won't be any other widgets to parent it to.
Well, the plugin-in-content thing (covered by other bugs) doesn't seem to be strictly needed here, since we do want d&d to preserve the frame tree long-term.  We just didn't do it as a first cut because it introduced so much complexity.
Why do we want d&d to preserve the frame tree?
The main reasons would be to avoid the cost of the frame teardown and reconstruct and to not lose scroll positions.  We might be able to do the latter with frame state restoration, but I'm not sure it actually works very well for anything other than the root scrollframe...
We can attach the scroll position to the content element as a property.

Is the cost of frame teardown and reconstruct worth the complexity?
Right now, no, which is why we do it that way.  If views and most subwidgets went away, so that the only complexity were to be reparenting of plug-in widgets, which would be necessary even if we don't keep the frame tree, then seems like it would be.  Yes, that's a big change.  So's moving plug-ins into content...
Duplicate of this bug: 470883
Duplicate of this bug: 472133
Blocks: 494931
Duplicate of this bug: 510638
Flags: wanted1.9.1?
I'm getting "File does not begin with '%PDF-'" when I drag a tab displaying a pdf off to it's own window.  Is this related
Very likely, yes, if the temp file gets deleted when the plugin instance is destroyed.
Duplicate of this bug: 524073
what about preserving plugin state also for session restore? is this desirable?
It may be desirable, but it's definitely a different bug than this one, and a much more challenging one: it requires cooperation from plugins to serialize and deserialize their state correctly, including references to DOM objects that will have new addresses in the new process.
Duplicate of this bug: 538381
Whiteboard: parity-chrome
Is there any chance of getting this for Firefox 4?  I would imagine most of the people who would know how to do this are busy with more important stuff :-(
This may be needed to support app-tabs, where a single app-tab can be visible in multiple browser windows. We would implement that by moving the real tab to the focused window and replacing the old tab with a fake screenshot.
Is that really the behaviour that we want?  It seems like we should grey it out or something, at the least, since otherwise it won't be clear which one is most up to date if they're both visible.  (Which window has focus is not always very obvious.)
We can keep the fake snapshot up to date (apart from windowed plugins). Or we could keep it static and gray it out. That's up to the UI folks. We still need to fix this bug.
(In reply to comment #27)
> It seems like we should grey it out

Yea, that's the current thinking. This bug doesn't affect doing that though.
Actually it seems to me swapping the frame trees might be a better approach for tab dragging and app-tabs. Post bug 130078, it's probably quite easy, and it would definitely be faster.

Boris, is there a bug for that?
Blocks: 566510
No longer blocks: 566510
Blocking final? This bug kinda sets back tear-off tabs a bit.
Duplicate of this bug: 585388
I don't see a chance of this landing until electrolysis comes to Firefox. 
I'm assuming that's how the rest of the browsers do it. Does Opera have process separation?
This has almost nothing to do with e10s.
(In reply to comment #33)
> I don't see a chance of this landing until electrolysis comes to Firefox. 
> I'm assuming that's how the rest of the browsers do it. Does Opera have process
> separation?

Don't make assumptions about things that you don't understand. Bugzilla is a bug tracker not a forum. Making these types of comments that also don't help developers fix the bug is putting yourself on the track to getting banned.
While this would be nice to fix, I think we have bigger fish to fry for 2.0...
blocking2.0: ? → -
We need this for app tabs (bug 551849), which is a major planned feature for Firefox 4, and it would be highly desired for tab multi-selection (bug 566510).

Also, roc noted in comment #30 that after fixing bug 130078, which is blocking betaN, fixing this bug will be easy.
blocking2.0: - → ?
I think we should swap the frame trees for FF4.
Per the platform meeting yesterday, this needs to block. roc, can you assign?
blocking2.0: ? → betaN+
status2.0: ? → ---
Mats, can you take this? This needs to be top priority.

The idea is to make nsSubdocumentFrame::Begin/EndSwapDocShells and the related code that actually swaps docshells swap the contained frame tree from one viewer to the other. Root views and plugin widgets will have to be reparented. But it seems to me it shouldn't be too hard.
Assignee: nobody → matspal
If we swap the frame trees, does this still depend on bug 337801?
I looked briefly at the code - preventing the presentation stuff from
being destroyed seems fairly easy; I don't understand how to fix the
root view/widget though, it has a null GetParent().
Where should it be hooked in?
I think we should assume bug 130078 has landed and fix this on top of that.
I have a patch that works smoothly on Linux. (on top of bug 130078.)
Attached patch Patch rev. 1 (obsolete) — Splinter Review
This patch should be applied on top of the bug 130078 patches: "hookup-vms",
"hookup-vms-2" and "imp-remove-widget".  I've tested it locally on Linux64,
OSX and Windows XP, both IPP and OOPP (not all combinations though).

TryServer builds:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mpalmgren@mozilla.com-c598140a3ac9/

Known problem: Print Preview is broken, it appears to be a regression
of bug 130078 (handled in bug 584193).
Attachment #469874 - Flags: review?(roc)
Depends on: 130078
Removing dependency as per comment 42; we've changed strategies in this bug such that we no longer depend on bug 337801
No longer depends on: 337801
+    FrameProperties props = aFrame->Properties();
+    PRBool found;
+    void* prop = props.Remove(DisplayItemDataProperty(), &found);
+    if (found) {
+      InternalDestroyDisplayItemData(aFrame, prop, PR_TRUE);
+    }

This can just be aFrame->Properties().Delete(DisplayItemDataProperty()).

Why is it necessary to hide the BaseWindows and then show them again?

+  mozilla::FrameLayerBuilder::DestroyDisplayItemDataFor(aFrame);

Just use "using namespace mozilla;"

+        if (nsGkAtoms::placeholderFrame == child->GetType()) {
+          nsIFrame* outOfFlowFrame =
+            nsPlaceholderFrame::GetRealFrameForPlaceholder(child);
+          do {
+            DestroyDisplayItemDataForFrames(outOfFlowFrame);
+          } while ((outOfFlowFrame = outOfFlowFrame->GetNextContinuation()));
+        }
+        else {
+          DestroyDisplayItemDataForFrames(child);
+        }

I don't think you need the placeholderFrame case here. We'll find all the frames on some list.

The rest looks great!
Actually, a few other thoughts:
-- the resize to 0,0 shouldn't be necessary, I would have thought. If you found it necessary on GTK, maybe something should be changed in nsWindow::SetParent.
-- what triggers a resize reflow when a docshell is swapped into a container with a different size?
-- needs tests!
Attached patch Patch rev. 2 (obsolete) — Splinter Review
(In reply to comment #48)
> This can just be aFrame->Properties().Delete(DisplayItemDataProperty()).

Fixed.

> Why is it necessary to hide the BaseWindows and then show them again?

It's not, I removed it.  What if DocumentViewerImpl::mPreviousViewer
is non-null though, could that cause problems?

> Just use "using namespace mozilla;"

Fixed.

> I don't think you need the placeholderFrame case here. We'll find all the
> frames on some list.

Right, when starting from the root frame that holds (otherwise not).
Fixed.


(In reply to comment #49)
> Actually, a few other thoughts:
> -- the resize to 0,0 shouldn't be necessary, I would have thought.

You're right, it was just a bug in nsWindow::SetParent on GTK.

> -- what triggers a resize reflow when a docshell is swapped into a container
> with a different size?

This does:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp#818

> -- needs tests!

Fixed.
Attachment #470799 - Flags: review?(roc)
Attachment #469874 - Attachment is obsolete: true
Attachment #469874 - Flags: review?(roc)
+  if (rootFrame)
+    ::DestroyDisplayItemDataForFrames(rootFrame);

{}

+    if (doc)
+      ::BeginSwapDocShellsForDocument(doc, nsnull);

{}

+    if (doc)
+      ::EndSwapDocShellsForDocument(doc, nsnull);

{}

> What if DocumentViewerImpl::mPreviousViewer is non-null though, could that
> cause problems?

Yes, but actually the problem is worse than that. The issue is that *every* viewer in the docshell, including bfcached documents, needs to have its presentation swapped. I think we need to enumerate all view children of the innerView of the subdocument frame.
Attached patch Part 1, rev. 3Splinter Review
Changes from last version:
 * made nsSubDocumentFrame process all the child views
 * fixed the nits
 * rewrote the test
Attachment #470799 - Attachment is obsolete: true
Attachment #471181 - Flags: review?(roc)
Attachment #470799 - Flags: review?(roc)
Comment on attachment 471181 [details] [diff] [review]
Part 1, rev. 3

The code looks good.

I think you should modify your test so that tabs are swapped to another window. That way we exercise widget reparenting.

I assume you tested that this works correctly with bfcached documents now? It would actually be good if you could add that to your test somehow. I think we should test that you can load document A in tab T, then navigate to document B, then swap T to another window, then go back in T to document A, and then dispatching an event to T using sendMouseEvent is received correctly in document A. (If the presentation was not hooked up correctly, then I expect A would not receive the event.)
Attachment #471181 - Flags: review?(roc) → review+
In fact you should probably test event dispatch for every document that you swap to a new window.
Hmm, this patch might break accessibility. We probably need to at least flush the accessibility trees for the swapped presentations. How can we do that?
In general while plugin window HWND and DOM tree are preserved then nothing that should be changed comes to my mind. Though we can't test the patch since accessibility is broken on trunk (bug 591874).
Added a test that opens a tab in a new window (replaceTabWithWindow),
then navigates back in that tab, then test synthesizeMouse clicking.
Added a few click tests to the previous same-window tab swapping as well.
Ok, I'm in the landing queue.

I investigated the a11y situation a bit.  As we reparent the views
we recursively do nsViewManager::ReparentWidgets which calls SetParent
on child widgets.  If there is a a11y node for the content document then
those shouldn't be a problem (assuming they are descendants of that).
For top-level widgets we might need to reparent the corresponding
a11y node. (not sure where those are stored in the a11y tree)
(BTW, on GTK2 it's also stored in nsWindow::mRootAccessible).
Remember that subdocuments don't have widgets now. nsViewManager::ReparentWidgets isn't actually going to do anything because it won't find any widgets. We should actually remove it!
Right, I was thinking of the widget for popups, combobox dropdown etc.
we may need to deal with a11y nodes for those, but let's do so in bug 591874.

Filed bug 593365 on removing nsViewManager::ReparentWidgets

Landed the patch briefly, but I had to backout since the added testcase
caused some other test later in the browser-chrome suite to fail.
Investigating...
Did you forget to close your window?

Hmm, I guess ReparentWidgets does still have to reparent widgets for popups. Sigh.
The first sign of a problem actually occurs before my test runs
so there's something missing/wrong with the code.  The error message is:
TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_bug561623.js | Console message: [JavaScript Error: "FullZoom.onLocationChange is not a function" {file: "chrome://browser/content/browser.js" line: 8402}]

FullZoom.onLocationChange is null, here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4413
and here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4573
Attached patch Part 2, rev. 1Splinter Review
There's an existing bug in nsWindow::SetParent on GTK2 - it doesn't add
the reparented child widget to the new parent's child list.

This doesn't fix the --browser-chrome failure though.  I believe the
cause of that is due to a recent regression on trunk which made the
patch here fail.  The symptom of that regression is
"FullZoom.onLocationChange is not a function", which is filed as bug 593378.
I'll add additional comments there.

I have a WIP for reparenting the native widget for popups,
just need to test it on more platforms.
Attachment #472658 - Flags: review?(roc)
(In reply to comment #65)
> I have a WIP for reparenting the native widget for popups,
> just need to test it on more platforms.

Can you add that to your test?
Depends on: 593378
Attached patch Part 3, rev. 1 (obsolete) — Splinter Review
nsThebesDeviceContext::mWidget is intialized from FindContainerView()
when a document viewer initialized.  We need to update it (or get rid of it).
http://mxr.mozilla.org/mozilla-central/source/gfx/src/thebes/nsThebesDeviceContext.h#162
Attachment #473812 - Flags: review?(roc)
Attachment #471532 - Attachment description: Patch rev. 4 (test changes only) → Part 1, rev. 4 (test changes only)
Attachment #471181 - Attachment description: Patch rev. 3 → Part 1, rev. 3
Comment on attachment 473812 [details] [diff] [review]
Part 3, rev. 1

What problems did you find that this fixes?

Are we ready to land now?
Attachment #473812 - Flags: review?(roc) → review+
(In reply to comment #68)
> Comment on attachment 473812 [details] [diff] [review]
> Part 3, rev. 1
> 
> What problems did you find that this fixes?

Moving a tab containing a combobox, then closing the original window
destroys that mWidget.  Clicking on the combobox crashed when it
tried to get the screen size through the device context, in
nsFormControlFrame::GetUsableScreenRect().

> Are we ready to land now?

Nope, still working on native widget reparenting on GTK2, which was
harder than I first thought.  I'm hoping to have a patch tomorrow.
Attached patch Part 3, rev. 2 (obsolete) — Splinter Review
It turned out 'mInitialized' is DEBUG only.  This version removes it
and uses mScreenManager != nsnull to test if we are initialized.
No other changes.
Attachment #473812 - Attachment is obsolete: true
Attachment #474470 - Flags: review?(roc)
Attached patch Part 4, rev. 1Splinter Review
Add nsIWidget::ReparentNativeWidget() to handle reparenting of
top-level widgets.
Attachment #474471 - Flags: review?(roc)
Attached patch Part 5, rev. 1Splinter Review
Add a test that creates child-widget popups (panel.openPopupAtScreen)
and synthesize events on them, then detach the tab (close the
original window) and finally run the tests again in the new window.
Strictly only to trigger assertions or crashes.
The patch set still doesn't pass browser-chrome suite though, I'm working
on that.  I noted that there are some existing problems with autocomplete
popups for form fill (bug 477797).  Probably not related to the test
failure though...
Depends on: 596022
Attached patch Part 6, rev. 1 (obsolete) — Splinter Review
Ok, so I found another broken test (bug 596022) and eventually a bug
in nsISessionStore::setWindowState() that affects closed windows that
were created by replaceTabWithWindow(), such as my tests do here.
Filed bug 596592.  Here's a temporary workaround for my tests.

With this workaround, the patch set pass unit tests without causing
test failures.  There's still a problem with fairly consistent chrome and
browser-chrome leaks reported for the tryserver runs I've done so far.
Clicking the "Analyze the leak" link, it says: leaked 10-12 DOMWINDOW(s).
I have no clue even where to start on that one.
Suggestions anyone?
Hmm, there's a recent leak on trunk as well, bug 596603...
Comment on attachment 475510 [details] [diff] [review]
Part 6, rev. 1

The workaround in bug 596592 is better.
Attachment #475510 - Attachment is obsolete: true
Attached patch Part 3, rev. 3Splinter Review
Doh. Fix the leak. Here's the intradiff:
< +        nsPresContext* pc = nsnull;
< +        dv->GetPresContext(&pc);
---
> +        nsCOMPtr<nsPresContext> pc;
> +        dv->GetPresContext(getter_AddRefs(pc));
Attachment #474470 - Attachment is obsolete: true
Attachment #476221 - Flags: review?(roc)
Attached patch Part 6, rev. 1Splinter Review
Add an empty PuppetWidget::ReparentNativeWidget() method to make it compile.
It will never be called AFAIU.
Attachment #476222 - Flags: review?(roc)
I've submitted the above patches + the various test fixes to the TryServer.
Should be ready to land as soon as the test results come in.
Blocks: 597979
filed bug 597979 : Preserve plugin state in session restore, per comments 21-22.
Depends on: 598294
Depends on: 605928
Duplicate of this bug: 615142
Depends on: 647392
Duplicate of this bug: 654230
Duplicate of this bug: 662249
Depends on: 732892
Depends on: 775082
Regressions: 1562827
You need to log in before you can comment on or make changes to this bug.