Closed Bug 610670 Opened 9 years ago Closed 9 years ago

Reuse a single puppet widget

Categories

(Firefox for Android Graveyard :: General, defect)

Other
Other
defect
Not set

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: anamaria.moldovan, Assigned: azakai)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [vkb])

Attachments

(1 file, 7 obsolete files)

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101108 Firefox/4.0b8pre
Build Identifier: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b8pre) Gecko/20101109 Firefox/4.0b8pre Fennec /4.0b3pre

Google search field becomes inactive after pressing the system Back button. 

Devices: Motorola DROID 2, HTC Desire, Samsung Galaxy S Captivated.

Reproducible: Always

Steps to Reproduce:
1. Visit www.google.com
2. Type in any phrase you wish.
3. Tap on the "I'm Feeling Lucky" button.
4. Press the system "back" button.
5. Tap in the input text field again. The VKB pops up.

Actual Results:  
The input text field is highlighted but one is no longer able to type in anything.

Expected Results:  
One should be able to type in anything.

Note: 

1. Same behavior is observed if after performing a Google search, one presses the system "back".
2. This is also reproducible on "http://search.yahoo.com/".
Build ID: 
Mozilla /5.0 (Android;Linux armv7l;rv:2.0b8pre) Gecko/20101115 Firefox/4.0b8pre Fennec /4.0b3pre

This is also reproducible under the following scenario :

1. Go to http://www-archive.mozilla.org/quality/browser/front-end/testcases/wallet/login.html (the Single Signon test Login Page.)     
2. Type in the following: user: "bar", password "foo"
3. Click on "Login"
4. When the confirm dialog appears, choose the "Remember" option
5. Tap back (from the right site panel)
6. Try to change the username and/or password.

Actual results:
One can delete the previous username/password but cannot type in anymore.

Expected results:
One should be able to type in.
tracking-fennec: --- → ?
Flags: in-testsuite?
Flags: in-litmus?
tracking-fennec: ? → 2.0+
I can't reproduce this (with the STR in comment #1)
Status: NEW → RESOLVED
tracking-fennec: 2.0+ → ?
Closed: 9 years ago
Resolution: --- → WORKSFORME
I can still reproduce it following the STR in comment #1. 

Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b8pre)
Gecko/20101125 Firefox/4.0b8pre Fennec /4.0b3pre

Please tell me if you need a video in order for everything to be more clear.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(In reply to comment #3)
> I can still reproduce it following the STR in comment #1. 
> 
> Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b8pre)
> Gecko/20101125 Firefox/4.0b8pre Fennec /4.0b3pre
> 
> Please tell me if you need a video in order for everything to be more clear.

Yes, that would be helpful.
Here you have the video: http://www.youtube.com/watch?v=6XjccNioieY

Build ID: Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b8pre)
Gecko/20101128 Firefox/4.0b8pre Fennec /4.0b3pre

Device: HTC Desire
Wes - let's find out what's happening here
Assignee: nobody → wjohnston
tracking-fennec: ? → 2.0+
Google's regular page doesn't do this for me, perhaps something to do with Google Instant.

However I can reproduce it by visiting google.com/m, doing a search, pressing back and trying to type.
Assignee: wjohnston → azakai
The problem is that after pressing back there is no longer a focused DOM window, this return will be run:

    if (NS_IsEventTargetedAtFocusedWindow(aEvent)) {
      nsCOMPtr<nsPIDOMWindow> window = GetFocusedDOMWindowInOurWindow();
      // No DOM window in same top level window has not been focused yet,
      // discard the events.
      if (!window) {
        return NS_OK;
      }
      ...

Any ideas on what the proper fix would be? Frontend or platform?
Summary: Google search field becomes inactive after pressing the system Back button. → Cannot enter text after pressing the system Back button
What would we set focus too? Just the browser? browser.active = true?
Does this mean tapping into the web content will make typing work?
Hmm, tapping or anything else doesn't seem to fix it. The only way to get out of the bad situation is to reload the page. So maybe there is some underlying problem with setting the focus.
After debugging, the issue turns out to be that when going back to a page in the bfcache, the child widget remains the old one.

So, what happens is you press back, but the child widget remains that of the page you just left. That page is frozen in the bfcache so typing etc. will not work, and probably other stuff we haven't noticed. The page looks correct visually though, which is confusing.

This stuff works when pages are not in the bfcache, since when new widgets are created, they are properly set as child widgets where they are needed. That is currently the only place in which the child widget is changed - when a new widget is created.

cc'ing cjones, who had some ideas on IRC about paths forward.
The reason gfx continues to work (misleadingly) is because of the way IPC layers are set up.  It's not terribly relevant to this bug, and was masking the worse problems.

So, in an ideal world, we would have one widget per TabChild, permanently.  One option for fixing this problem is to get to that ideal implementation.  In the current setup, we have an hierarchy that looks something like

  TabChild
    PuppetWidget (toplevel, permanent)
    nsWebBrowser
      (uses toplevel PuppetWidget)
      [other stuff]
        DocumentViewerImpl (per document)
          PuppetWidget (child, supposed to be the only active child)

With bfcache, we get additional

        DocumentViewerImpl (per document)
          PuppetWidget (child, supposed to be the only active child)
        DocumentViewerImpl (per document)
          PuppetWidget (child, supposed to be the only active child)

and the bug is that "supposed to be the only active child" is being broken, as Alon found (thanks!).

In the ideal world, we would instead have

  TabChild
    PuppetWidget (toplevel, permanent)
    nsWebBrowser
      (uses toplevel PuppetWidget)
      [other stuff]
        DocumentViewerImpl (per document)
          (uses toplevel PuppetWidget)

that is, the DocumentViewerImpls would all grab references to the toplevel widget owned by TabChild, instead of creating new ones.  We have code in place for this, because we needed it to draw in titlebar (IIRC) on windows desktop FF.  That's all set up here

  http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#2249

The "right" solution to this bug is to make that code work for PuppetWidgets, in content processes.  That is, only create one PuppetWidget per TabChild, and reuse that everywhere in subordinate contexts.  I punted on doing that originally because we were under tight time constraints.

Another solution that might be more expedient in the short term, but wasted work in the long term, is to use nsIWidget state to update the "current child" in the PuppetWidget backend.  nsIWidgets track whether they're active and visible.  We could figure out the right magic state changes to tell us how to update the child.

Since this is a blocking-final bug, I lean towards attempting the "right" solution.

Does that all make sense?
(CC'ing you guys to see if you agree with comment 13.)
Maybe I'm reading the above too literally, but DocumentViewerImpl::MakeWindow is not even called when pressing the back button. So I don't see how making the code there work for puppet widgets in content processes would help?
If MakeWindow() re-used the "top-level" PuppetWidget instead of creating its own child, we wouldn't need child PuppetWidgets at all and this bug would disappear.  In other words, there would be no need to set a "current PuppetWidget child" on back/forward because there would be no child PuppetWidgets.
Sounds good to me too.
(In reply to comment #13)
> In the
> current setup, we have an hierarchy that looks something like
> 
>   TabChild
>     PuppetWidget (toplevel, permanent)
>     nsWebBrowser
>       (uses toplevel PuppetWidget)
>       [other stuff]
>         DocumentViewerImpl (per document)
>           PuppetWidget (child, supposed to be the only active child)
> 
> With bfcache, we get additional
> 
>         DocumentViewerImpl (per document)
>           PuppetWidget (child, supposed to be the only active child)
>         DocumentViewerImpl (per document)
>           PuppetWidget (child, supposed to be the only active child)
> 
> and the bug is that "supposed to be the only active child" is being broken

What does 'active' mean here? Pages in the bfcache are frozen, so I guess some other meaning is intended?

Also, my understanding is that the bug is that the child widget is not updated when it should - it's only updated when a new one is created, not when one is browsed to; 'activity' doesn't seem to factor into that. I don't quite see how the two descriptions of the bug match up so I guess I am misunderstanding something.
"Active" means the puppetwidget that produces gfx and receives events.  More specifically, it's toplevelPuppetWidget.mChild.  The bug is that mChild isn't updated when navigating to pages in bfcache.

These questions would be moot if we eliminated PuppetWidget.mChild ;).  I think that's still worth attempting.

CC'ing jimm, who did the windows version of this.
Playing around with reusing the toplevel widget, the first issue is that nothing is rendered in the window. It looks like the problem is as follows.

When a widget is associated with a view (which for us here, happens after calling CreateChild), then SetClientData() is called. However, if a single widget is reused for multiple views (one toplevel + multiple others), then SetClientData ends up being called multiple times, overwriting the previous value if any. In particular a view is added, then destroyed, so the clientData is set to 0, and this causes invalidate events to not be handled, and nothing is rendered.

Even if somehow the toplevel one can be ignored, we still have multiple others. I'm starting to worry that fixing this is more complicated than the benefit it gives (for example, a potential 'fix' might be by letting a widget have multiple clientDatas - ugh). Or is there a good solution here that I miss?
On Windows we attach to an existing parent widget instead of trying to reuse an existing widget. See http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#2316 We use the 'attachment' to allow more than one view to share the same widget, but only one view actualy owns the widget and gets put in the widget's client data. The other ones just "attach".
Thanks for the info.

Ok, trying to emulate that approach for puppet widgets, it seems the first issue is that the event handler function is not set. This is because TabChild sets it to 0 when creating the toplevel widget, and it is never set later if we attach to an existing widget (which has 0 as the handler).

Is this an issue due to something special with puppet widgets in TabChild? Or is there a special solution for it on Windows?
The event handler is null for the "toplevel" puppetwidget because in the current setup, when the view system creates a child of that toplevel puppetwidget, it sets the child's event handler here

http://mxr.mozilla.org/mozilla-central/source/view/src/nsView.cpp#758

I must confess to not knowing how this works for windows.  It looks like we'd end up here

http://mxr.mozilla.org/mozilla-central/source/view/src/nsView.cpp#855

and set the view callback to ::AttachedHandleEvent.  If that's all working properly, then puppetwidget probably needs logic in its event-dispatch code to deal with having nsIWidget::mViewCallback set.
After telling it to attach to the parent widget, then hackishly setting the event handler to the valid value fixes painting. However loading pages from the bfcache is broken - the page doesn't load (but pressing reload will show it).

Setting mViewCallback doesn't seem to help with that.
Looks like reloading bfcache pages is broken since |nsViewManager::UpdateViewNoSuppression|'s |displayRoot| returns NULL from |GetWidget|. Or at least that is where things diverge between when it works and when not.
It looks like we need to get a AttachToTopLevelWidget call to set the widget on the view, does that happen when a document comes out of the bfcache?
Seems like AttachToTopLevelWidget is only called when non-bfcache pages are loaded.

However, that would also be the case on Windows, I presume? But there is no special call to AttachToTopLevelWidget there.
I'm not sure. Either that one call happens for bfcached documents, or the widget pointer on the view doesn't get nulled out.
The widget pointer gets nulled out in |nsIView::AttachToTopLevelWidget|. It checks if there is already a view attached to the widget, if so, that view is detached.

That is, after we attach to the widget and browse to another page, we are detached from the widget and the current page attaches. But when we return, as mentioned above |AttachToTopLevelWidget| is not called, and we have no widget.

(Naively removing the nulling out of the pointer breaks rendering of pages.)
Sounds like AttachToTopLevelWidget must get called on Windows when we bring a document out of the bfcache. Does the MakeWindow call in DocumentViewerImpl::Show accomplish that?
Looks like MakeWindow isn't called from Show, since the condition

  if (mDocument && !mPresShell) {

isn't hit.
Changing the condition in various ways doesn't seem to lead to anything useful.

I don't see any Windows-specific code here, or that calls here. How does this end up working on Windows?
I'm not sure. Can you get it in a debugger on Windows and see how it works?
(In reply to comment #30)
> That is, after we attach to the widget and browse to another page, we are
> detached from the widget and the current page attaches. But when we return, as
> mentioned above |AttachToTopLevelWidget| is not called, and we have no widget.
> 
> (Naively removing the nulling out of the pointer breaks rendering of pages.)

We only have one top level widget now on windows. MakeWindow originally created and discarded child widgets but we fixed that. It now reuses the top level widget so there's no swapping out taking place.
(In reply to comment #34)
> I'm not sure. Can you get it in a debugger on Windows and see how it works?

I don't have a Windows machine, I am afraid, so that would be time-consuming for me to arrange. Unless there is no other way ;)

(In reply to comment #35)
> We only have one top level widget now on windows. MakeWindow originally created
> and discarded child widgets but we fixed that. It now reuses the top level
> widget so there's no swapping out taking place.

Yes, exactly, I am using the same code in MakeWindow that runs on Windows, for this. However, I am stuck on the following issue, that I don't understand how it works on Windows: With a single reused widget, AttachToTopLevelWidget is not called when browsing to pages *in the bfcache*. That means that there is no widget for bfcache pages we browse back to.

How does Windows handle this? I can't seem to find a special call to AttachToTopLevelWidget (or something that eventually gets there) that only happens on Windows. Is there some special bfcache trick that is done?

Other question, which bug is the Windows code in?
Hmm, I haven't worked with bfcache before, you might have found an unexposed bug. How would I force a xul doc into and out of the cache? Maybe through a xulrunner app? FWIW, we didn't have any issues with cached pages when we implemented this. (bug 513162)
Well, for normal HTML pages on the web, just browsing to a page and pressing back will load the previous one from the bfcache (and run into the problem I mentioned).

Maybe I am misreading your comment, but does the Windows code for this widget stuff handle xul pages differently than web content? (our problem here is with web content, not xul pages)
(In reply to comment #38)
> Well, for normal HTML pages on the web, just browsing to a page and pressing
> back will load the previous one from the bfcache (and run into the problem I
> mentioned).
> 
> Maybe I am misreading your comment, but does the Windows code for this widget
> stuff handle xul pages differently than web content? (our problem here is with
> web content, not xul pages)

We use a single Windows window/widget for each Fx window. Web content (tabs) are contained in widgetless views. (Bug 130078 removed tab widgets) AttachToTopLevelWidget only applies to these top level 'physical' windows.

Before all of this, we had a funny widget hierarchy on windows:

top level nsXULWindow widget
 - inner child widget owned by document viewer (browser chrome)
   - child widget nsXULWindow
     - inner child tab widget owned by document viewer (content)
 
Now, it's just a top level nsXULWindow w/ document viewer calling AttachToTopLevelWidget on the widget.
(In reply to comment #31)
> Sounds like AttachToTopLevelWidget must get called on Windows when we bring a
> document out of the bfcache. Does the MakeWindow call in
> DocumentViewerImpl::Show accomplish that?

This isn't needed on windows, but it's probably what we are missing on fennec to finish up this work?
I'm also wondering if when we switch to using remote content on windows, this will need to be fixed as well. I'm not familiar enough with our remote content work to know for sure, will tabs in Fx become PuppetWidgets some day, or do we have a way to remote widgetless views?
(In reply to comment #40)
> (In reply to comment #31)
> > Sounds like AttachToTopLevelWidget must get called on Windows when we bring a
> > document out of the bfcache. Does the MakeWindow call in
> > DocumentViewerImpl::Show accomplish that?
> 
> This isn't needed on windows, but it's probably what we are missing on fennec
> to finish up this work?

Why isn't this needed on Windows?
(In reply to comment #41)
> I'm also wondering if when we switch to using remote content on windows, this
> will need to be fixed as well. I'm not familiar enough with our remote content
> work to know for sure, will tabs in Fx become PuppetWidgets some day, or do we
> have a way to remote widgetless views?

Yes, the code Alon is hacking on will be used on all platforms with OOP content.  PuppetWidgets only live in content processes, and aren't platform specific.
(In reply to comment #42)
> (In reply to comment #40)
> > (In reply to comment #31)
> > > Sounds like AttachToTopLevelWidget must get called on Windows when we bring a
> > > document out of the bfcache. Does the MakeWindow call in
> > > DocumentViewerImpl::Show accomplish that?
> > 
> > This isn't needed on windows, but it's probably what we are missing on fennec
> > to finish up this work?
> 
> Why isn't this needed on Windows?

Well, just poking through the code.. when a page comes out of history, DocumentViewerImpl::Open gets called. mParentWidget for this viewer is null (There's no 'physical' widget associated with the tab) so there's no widget to attach to.
> 
> Well, just poking through the code.. when a page comes out of history,
> DocumentViewerImpl::Open gets called. mParentWidget for this viewer is null
> (There's no 'physical' widget associated with the tab) so there's no widget to
> attach to.

Why isn't not having a widget for the tab a problem for Windows?

I am using the Windows-specific code in DocumentViewerImpl for Puppet Widgets in my tests, and also doing the Windows event forwarding stuff. I don't know the view and widget code almost at all, so perhaps I am missing something basic here, but given that, what is the remaining difference between Windows and Puppet Widgets?
(In reply to comment #45)
> > 
> > Well, just poking through the code.. when a page comes out of history,
> > DocumentViewerImpl::Open gets called. mParentWidget for this viewer is null
> > (There's no 'physical' widget associated with the tab) so there's no widget to
> > attach to.
> 
> Why isn't not having a widget for the tab a problem for Windows?

Well, clearly it works. :) What issue were you expecting it to have? Maybe I can help answer that.

> 
> I am using the Windows-specific code in DocumentViewerImpl for Puppet Widgets
> in my tests, and also doing the Windows event forwarding stuff. I don't know
> the view and widget code almost at all, so perhaps I am missing something basic
> here, but given that, what is the remaining difference between Windows and
> Puppet Widgets?

I haven't worked with fennec code so I really can't answer that.

Anyone know if content processes are running in the desktop version of fennec that runs on windows?
I guess I have some faulty assumptions. I thought that, prior to the recent change in Windows, it worked sort of like Puppet Widgets do now (new widget for each tab, plus a toplevel one). Then the recent work was done to reuse a single toplevel widget. So I tried to do that exact same change in Puppet Widgets, but it doesn't work. So I guess I have misunderstood something about the history here.

The issue I would expect Windows to have is this: A view gets its widget nulled out when that same widget is attached to another view. That happens when you browse to another page. So if we press back and try to return to that page, then the view has no widget, and it won't load. This is so because the Attach call only fires for new pages, not ones in the bfcache.

If special Windows code called Attach for bfcached pages, I would expect Windows to work. But I can't find code for that.

> Anyone know if content processes are running
> in the desktop version of fennec that runs on
> windows?

I am fairly sure they should work ok, but they don't get tested as constantly as other platforms.
(In reply to comment #46)
> Anyone know if content processes are running in the desktop version of fennec
> that runs on windows?

Currently broken by bug 617860, but fixing/working around that shouldn't be too terribly bad.
(In reply to comment #47)
> I guess I have some faulty assumptions. I thought that, prior to the recent
> change in Windows, it worked sort of like Puppet Widgets do now (new widget for
> each tab, plus a toplevel one). Then the recent work was done to reuse a single
> toplevel widget. So I tried to do that exact same change in Puppet Widgets, but
> it doesn't work. So I guess I have misunderstood something about the history
> here.
> 

Ok, I think I see the misunderstanding. Support for tab widgets was never implemented. The original attach code only applied to top level windows. Then bug 130078 landed removing those. So changes to support detatch/reattach for pages coming out of bfcache haven't been added yet.
Thanks, now I am starting to understand. So what is needed here is new functionality that doesn't exist yet in any other widget platform code.
Ah yes of course, the Fennec situation is unique because up until now the browser chrome has been the only document to attach to a widget, and it is never navigated. So you would need something new here. Sorry for not realizing earlier.
(In reply to comment #50)
> Thanks, now I am starting to understand. So what is needed here is new
> functionality that doesn't exist yet in any other widget platform code.

Yep. Although will it require widget platform changes? Seems like a little widget/view glue in document viewer's Load and PageHide methods might suffice.
(In reply to comment #52)
> (In reply to comment #50)
> > Thanks, now I am starting to understand. So what is needed here is new
> > functionality that doesn't exist yet in any other widget platform code.
> 
> Yep. Although will it require widget platform changes? Seems like a little
> widget/view glue in document viewer's Load and PageHide methods might suffice.

* Open rather than Load.
Adding a forced call to MakeWindow is not enough. A view is created and attached, but it isn't linked properly to the frame, so when invalidate is called, it happens on the old view, which has no widget.

Looks like a new frame might need to be created as well. Does that make sense, or is creating a new frame obviously wrong for some reason? (performance, correctness, etc.)
Creating a new frame would throw away and recreate the whole frame tree, and isn't saving the frame tree one of the main points of the bfcache?
Can you just reuse the old view?
I'll try to find out how to reuse the view. There doesn't seem to be an obvious way to do it (SetView() is normally just called when creating a new frame).
Isn't the old view still associated with the root frame? Can't we just attach that view to the existing widget?
I think I finally got it to attach properly. Browsing to pages in the bfcache now appears to work. However, this has broken returning to non-bfcache pages in the history... I'll start to debug that, but judging from the time spent so far, I am not confident that this can be done in time for final.
You know that non-bfcached documents were broken by your changes, so it shouldn't be as hard as fixing bfcached documents. Do you want to post a work in progress patch of what you have?
Attached patch ugly wip patch (obsolete) — Splinter Review
Yeah, I'm hoping it'll be easier than the work so far. However, in this bug I just don't have a clear idea of what is wrong and how much work is left, so wanted to give warning.

Here is the current patch I'm using. Sorry for the ugliness, it's just hacked together.
No progress so far in figuring this out. Two questions:

1. A new DocumentViewerImpl is created when we load a page by going back to it in the history (if it isn't in the bfcache). Is that how we want things to work, with the changes we want done in this bug?

2. The current problem is that, in pages loaded from history (but not bfcache), the titlebar is updated to the proper page, but the contents of the page remain the same as before. What would be the proper place to debug this? I am looking basically randomly at things that seem relevant to me, looking for something going wrong, but it's almost like looking for a needle in a haystack.
(In reply to comment #62)
> 1. A new DocumentViewerImpl is created when we load a page by going back to it
> in the history (if it isn't in the bfcache). Is that how we want things to
> work, with the changes we want done in this bug?

I think so.

> 2. The current problem is that, in pages loaded from history (but not bfcache),
> the titlebar is updated to the proper page, but the contents of the page remain
> the same as before. What would be the proper place to debug this? I am looking
> basically randomly at things that seem relevant to me, looking for something
> going wrong, but it's almost like looking for a needle in a haystack.

Is it a painting problem? ie does forcing repainting fix the problem? Is the presshell not getting set to active? Is the invalidate that usually redraws the page dieing because it doesn't get to a widget? Is the widget hooked up correctly?
> Is the
> presshell not getting set to active? Is the invalidate that usually redraws the
> page dieing because it doesn't get to a widget? Is the widget hooked up
> correctly?

Thanks for the directions. I guess this is just taking me a long time since I have not worked with these classes before.
It looks like when we load a page that isn't in the bfcache, we have an nsSHEntry for it but it has no content viewer. So nothing is rendered.

The nsSHEntry has no content viewer since DocumentViewerImpl::Show for another page calls EvictContentViewers. That clears the content viewer that we later need. More specifically, this is what happens:

1. Click to view page 1. It loads.
2. Click to view page 2. It loads.
3. Click to view page 3. We evict the SHEntry for page 1. Page 3 loads.
4. Click back. Page 2 appears.
5. Click back. Page 1 does *not* appear since SHEntry has no content viewer.

I am not sure what should be done here. Should the eviction not have happened, or should it be 'fixed' by setting the content viewer? Or should a new nsSHEntry have been created (should the evicted one have been dropped)?
> It looks like when we load a page that isn't in the bfcache, we have an
> nsSHEntry for it but it has no content viewer

Right.  If it's not in bfcache, there is no cached dom/presentation.  So then we go through the docshell code, kick off a load of the history entry's URI, and when that starts giving us data create a new document viewer (see nsDocShell::Embed and the like).

In that situation you should go through MakeWindow.  Do you hit the AttachToTopLevelWidget call there in that case?
(In reply to comment #66)
> In that situation you should go through MakeWindow.  Do you hit the
> AttachToTopLevelWidget call there in that case?

Yes, MakeWindow is reached, and it calls AttachToTopLevelWidget. That part seems ok.

Meanwhile I see that *without* this patch, the nsSHEntry has no content viewer when returning to a page in the history that is not in the bfcache, just like with this patch. But it renders properly somehow. Back to the debugger...
(In reply to comment #67)
> Meanwhile I see that *without* this patch, the nsSHEntry has no content viewer
> when returning to a page in the history that is not in the bfcache, just like
> with this patch. But it renders properly somehow. Back to the debugger...

So you need to create a new content viewer for the SHEntry. Presumably this happens correctly without your patch, why does it not happen with your patch?
Do we have evidence that it doesn't happen?  Comment 67 sounded like it does happen, and we even call MakeWindow on the newly-created viewer.
Ok, the specific issue here was that Hide is called on the shared widget, but Show isn't, due to

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#1946

(So if the widget wasn't shared, it would work since Hide was being called on another widget.) Making that code run in our case makes the page show up properly.

Stuff is no longer completely broken, so I'll start testing on device, to see if it fixes the original issue that led to this work.
Attached patch patch (obsolete) — Splinter Review
Here is a patch that seems to actually work. At least I don't see any obviously broken stuff, and the original issue with not being able to type after pressing back is fixed as well.

I'm not sure about some parts of the patch, like the |SetEventCallback| that I had to add. Thoughts?
Attachment #502822 - Attachment is obsolete: true
Attachment #506927 - Flags: feedback?(jones.chris.g)
Comment on attachment 506927 [details] [diff] [review]
patch

This looks mostly OK to me as far as I know this code, which is quite little.

This pattern

  if (XRE_GetProcessType() == GeckoProcessType_Content)
...

isn't what we want.  Instead, at least check

  nsIWidget::UsePuppetWidgets()

or better yet, factor out the #if WINDOZE and puppet widget gunk into a ShouldAttachToToplevel() or something.

feedback?ing tn, who can give better feedback.
Attachment #506927 - Flags: feedback?(tnikkel)
Attachment #506927 - Flags: feedback?(jones.chris.g)
Attachment #506927 - Flags: feedback+
Comment on attachment 506927 [details] [diff] [review]
patch

I can't review this patch, so I think it would be better to ask for feedback from someone who can: roc, dbaron, or bz I think.
Comment on attachment 506927 [details] [diff] [review]
patch

Ok, asking roc.
Attachment #506927 - Flags: feedback?(tnikkel) → feedback?(roc)
Comment on attachment 506927 [details] [diff] [review]
patch

bz is best for this (as usual :-) )
Attachment #506927 - Flags: feedback?(roc) → review?(bzbarsky)
Why do we need the attach in Open()?  Is that to handle the case of us coming out of bfcache?  Do we have an mAttachedToParent which is out of sync with our actual attachment state there or something?

See the comments in comment 72.

The code in MakeWindow looks wrong.  In particular, if XRE_GetProcessType() == GeckoProcessType_Content on Windows, it won't set mAttachedToParent to true, right?

I have no idea what the nsView change is needed for.  Get roc to look at that one, please?
+  // In content processes, we reuse a single widget, so we need
+  // to ensure the event handler function is properly set
+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
+    ((nsBaseWidget*)aWidget)->SetEventCallback(::HandleEvent);
+  }

Yeah, I don't know why this is needed either. We already call AttachViewToTopLevel here, so we should be getting events.
Attached patch patch (obsolete) — Splinter Review
Thanks for the feedback, here is an updated patch. Hopefully this is getting close to review-worthy material.

> Why do we need the attach in Open()?  Is that to handle
> the case of us coming out of bfcache?  Do we have an
> mAttachedToParent which is out of sync with our
> actual attachment state there or something?

The issue is that our shared widget will get detached when the previous page is left. This is needed for pages in the history (but not new pages that we browse to), which need the additional attachment call as in the patch.
Attachment #506927 - Attachment is obsolete: true
Attachment #507745 - Flags: feedback?(bzbarsky)
Attachment #506927 - Flags: review?(bzbarsky)
> The issue is that our shared widget will get detached

Detached from what?  Where does that happen?

> This is needed for pages in the history 

So the ones coming out of bfcache, right?  Again, do those end up with an mAttachedToParent that doesn't reflect reality?  Or should we be conditioning this attachment in Open() on mAttachedToParent, like the one in MakeWindow()?

Speaking of which, why do we need to call Show on the widget in the mAttachedToParent content-process case in Show(), but not in Hide()?
Attached patch patch (obsolete) — Splinter Review
> So the ones coming out of bfcache, right?  Again, do those end up with an
> mAttachedToParent that doesn't reflect reality?  Or should we be conditioning
> this attachment in Open() on mAttachedToParent, like the one in MakeWindow()?

Ok, here is a summary of the attachment stuff, which clarifies and corrects my previous comments:

When loading a new page, DocumentViewerImpl::MakeWindow calls nsIView::AttachToTopLevelWidget.

When going back into a non-bfcache page, we also call DocumentViewerImpl::MakeWindow, which calls nsIView::AttachToTopLevelWidget.

When going back into the bfcache, DocumentViewerImpl::MakeWindow is *not* called, so we need to add a call in DocumentViewerImpl::Open to attach.

I added code to properly track mAttachedToParent, which my previous patch didn't sync properly.

> 
> Speaking of which, why do we need to call Show on the widget in the
> mAttachedToParent content-process case in Show(), but not in Hide()?

I added the code in Show() since it doesn't work otherwise. I haven't in practice seen a need for adding parallel code to Hide(), but I don't really have a global understanding of everything going on here. Does it make sense to add code in Hide()?
Attachment #507745 - Attachment is obsolete: true
Attachment #507745 - Flags: feedback?(bzbarsky)
> When loading a new page, DocumentViewerImpl::MakeWindow calls
> nsIView::AttachToTopLevelWidget.

Yes, this part makes sense.

> When going back into the bfcache, DocumentViewerImpl::MakeWindow is *not*
> called, so we need to add a call in DocumentViewerImpl::Open to attach.

That's my question.  Why?  What detached us?

> I added the code in Show() since it doesn't work otherwise.

Well, clearly.  But why is it needed?  Why is the widget hidden there to start with?

> but I don't really have a global understanding of everything going on here

That's ok, because no one actually does.  ;)  roc and I pretend like we might.

> Does it make sense to add code in Hide()?

I don't know.  It depends on the widget setup involved here.  Chris?

Speaking of which, you should probably address Chris's comments too.
> > When going back into the bfcache, DocumentViewerImpl::MakeWindow is *not*
> > called, so we need to add a call in DocumentViewerImpl::Open to attach.
> 
> That's my question.  Why?  What detached us?

Turns out we haven't been detached. In fact, detachment only happens from the Attach() function (it detaches right before actually doing the attachment).

Attachment is important not because we were detached, but because of other stuff that happens in the Attach(). I'm not sure exactly what, if it's important I can check.

> 
> > I added the code in Show() since it doesn't work otherwise.
> 
> Well, clearly.  But why is it needed?  Why is the widget hidden there to start
> with?

It is hidden as part of the normal movement between one page and another. We first hide the previous one, then show the current one.

With a shared widget, we hide and show the same one. The issue is that there is a hide, but not a show, due to the

  if (!mAttachedToParent)

condition. Presumably that was added for Windows-specific reasons with shared widgets. In the content process+shared widgets, though, we need it.

> Speaking of which, you should probably address Chris's comments too.

I did the refactoring part cjones asked for, haven't had time to look into the |nsIWidget::UsePuppetWidgets| thing. I'll do that next.
Attached patch patch (obsolete) — Splinter Review
This patch addresses all the comments.
Attachment #507956 - Attachment is obsolete: true
> I'm not sure exactly what, if it's important I can check.

Yes, it's important.  We need to figure out why this is needed and document it.  This file has too much random "magic" already.  We really want to be _removing_ it, not adding it.

> We first hide the previous one, then show the current one.

Even with the puppet widget?  Where does that happen?

In any case, do we not need to hide the puppet widget when the document viewer is hidden?  Or is the point that if the widget is shared across viewers then hiding the viewer should not necessarily hide the widget?  Again, this is something that needs better documentation...
(In reply to comment #84)
> > I'm not sure exactly what, if it's important I can check.
> 
> Yes, it's important.  We need to figure out why this is needed and document it.
>  This file has too much random "magic" already.  We really want to be
> _removing_ it, not adding it.
> 

Ok, what happens is this:

1. We are at page A (and attached to it).
2. We browse to page B. This calls attach for that new page, and that does a detach from the previous page (we cannot have two attachments). So page A's view is now not attached to anything.
3. We press 'back' in order to revisit page A. It is loaded from the bfcache. No attach call is done, so we are now using A's view, which is attached to nothing, and so nothing is rendered.

Adding an attach call in ::Open as in the patch fixes the issue. I added this to the patch to better document things:

  // Attachment is necessary, since we get detached when another page
  // is browsed to. That is, if we are one page A, then when we go to
  // page B, we detach. So page A's view has no widget. If we then go
  // back to it, and it is in the bfcache, we will use that view, which
  // doesn't have a widget. The attach call here will properly attach us.


> > We first hide the previous one, then show the current one.
> 
> Even with the puppet widget?  Where does that happen?
> 

I don't think puppet widgets are special here. Anytime we browse to a new page,
a new viewer is created, and we call Hide through

  DocumentViewerImpl::Hide
  DocumentViewerImpl::InitInternal
  DocumentViewerImpl::Init
  nsDocShell::SetupNewViewer

and later Show is called through

  DocumentViewerImpl::Show
  nsPresContext::EnsureVisible
  PresShell::UnsuppressAndInvalidate
  PresShell::UnsuppressPainting
  DocumentViewerImpl::LoadComplete

The question I have is why the |if (!mAttachedToParent)| check is there - that is, why does Windows not need Show being called, unlike everything else. If Windows didn't have that special code, we wouldn't need special code for puppet widgets here (which we need, since they also attach to the parent, like Windows).

> In any case, do we not need to hide the puppet widget when the document viewer
> is hidden?  Or is the point that if the widget is shared across viewers then
> hiding the viewer should not necessarily hide the widget?

As I understand it, the widget is hidden with the document viewer. The only exception, again, is Windows (for reasons unknown to me). Puppet widgets are the normal case.
> I added this to the patch to better document things

Thank you.  That's much better.

So should we rename mAttachedToParent to mShouldBeAttachedToParent, since that's what it seems to mean?

> and we call Hide through

Yes, but that doesn't hide the puppet widget with your patch, because mAttachedToParent is true, right?  So again, what's hiding the puppet widget, if anything?

> The question I have is why the |if (!mAttachedToParent)| check is there 

Because if mAttachedToParent, we don't own the widget and shouldn't be messing with its visibility, last I checked.

> that is, why does Windows not need Show being called

Because the widget's visibility is controlled by whoever owns the widget.  In the Windows case before your patch, the attached case only happens for the toplevel window, and its visibility is handled by nsXULWindow and the like.  It shouldn't be affected by the xul loaded in the window.

With your patch, that still seems to be true; unless we create a new puppet widget on each page transition (doesn't sound like we do), its visibility seems like it shouldn't be tied to a particular document viewer.

> As I understand it, the widget is hidden with the document viewer.

Where does that happen?   It sure doesn't happen from DocumentViewerImpl::Hide.
Attached patch patch (obsolete) — Splinter Review
> So should we rename mAttachedToParent to mShouldBeAttachedToParent, since
> that's what it seems to mean?
>

Well, the member variable is actually used as 'now attached'. But in MakeWindow it is set before the actual attachment, which is done later conditionally. I added a local variable there for 'shouldAttach', so mAttachedToParent will now always indicate exactly when we are currently attached.

However, this potentially changes the behavior on Windows, in the case that the decision is made to attach, but attachment is not actually done later in MakeWindow. This seems like a bug which should be fixed anyhow, but fallout is possible.

> > and we call Hide through
> 
> Yes, but that doesn't hide the puppet widget with your patch, because
> mAttachedToParent is true, right?  So again, what's hiding the puppet widget,
> if anything?
> 

It's hidden from a reflow (which only happens on new pages - ones that are not in the history). Here is a stack trace, I left most of it since I am not sure what is relevant here:

#0  mozilla::widget::PuppetWidget::Show (this=0xaea71200, aState=0)
    at /home/alon/Dev/mozilla-central/widget/src/xpwidgets/PuppetWidget.cpp:171
#1  0xb5b6b4aa in nsView::NotifyEffectiveVisibilityChanged (this=0xae338a60, aEffectivelyVisible=0)
    at /home/alon/Dev/mozilla-central/view/src/nsView.cpp:513
#2  0xb5b6b513 in nsView::SetVisibility (this=0xae338a60, aVisibility=nsViewVisibility_kHide)
    at /home/alon/Dev/mozilla-central/view/src/nsView.cpp:529
#3  0xb5b71c1e in nsViewManager::SetViewVisibility (this=0xae9addd0, aView=0xae338a60, aVisible=nsViewVisibility_kHide)
    at /home/alon/Dev/mozilla-central/view/src/nsViewManager.cpp:1398
#4  0xb558f2d6 in nsComboboxControlFrame::ReflowDropdown (this=0xae372950, aPresContext=0xae9c1c00, aReflowState=...)
    at /home/alon/Dev/mozilla-central/layout/forms/nsComboboxControlFrame.cpp:455
#5  0xb558f9ed in nsComboboxControlFrame::Reflow (this=0xae372950, aPresContext=0xae9c1c00, aDesiredSize=..., aReflowState=..., 
    aStatus=@0xbf977348) at /home/alon/Dev/mozilla-central/layout/forms/nsComboboxControlFrame.cpp:643
[.. a lot more reflow stuff ..]
#42 0xb55b8765 in nsBlockFrame::Reflow (this=0xae33d440, aPresContext=0xae9c1c00, aMetrics=..., aReflowState=..., aStatus=@0xbf97c99c)
    at /home/alon/Dev/mozilla-central/layout/generic/nsBlockFrame.cpp:1080
#43 0xb55d9544 in nsContainerFrame::ReflowChild (this=0xae9dd010, aKidFrame=0xae33d440, aPresContext=0xae9c1c00, aDesiredSize=..., 
    aReflowState=..., aX=0, aY=0, aFlags=0, aStatus=@0xbf97c99c, aTracker=0x0)
    at /home/alon/Dev/mozilla-central/layout/generic/nsContainerFrame.cpp:740
#44 0xb5616468 in nsCanvasFrame::Reflow (this=0xae9dd010, aPresContext=0xae9c1c00, aDesiredSize=..., aReflowState=..., aStatus=@0xbf97c99c)
    at /home/alon/Dev/mozilla-central/layout/generic/nsCanvasFrame.cpp:494
#45 0xb55d9544 in nsContainerFrame::ReflowChild (this=0xae9dd190, aKidFrame=0xae9dd010, aPresContext=0xae9c1c00, aDesiredSize=..., 
    aReflowState=..., aX=0, aY=0, aFlags=3, aStatus=@0xbf97c99c, aTracker=0x0)
    at /home/alon/Dev/mozilla-central/layout/generic/nsContainerFrame.cpp:740
#46 0xb5605779 in nsHTMLScrollFrame::ReflowScrolledFrame (this=0xae9dd190, aState=0xbf97cb24, aAssumeHScroll=0, aAssumeVScroll=0, 
    aMetrics=0xbf97ca14, aFirstPass=1) at /home/alon/Dev/mozilla-central/layout/generic/nsGfxScrollFrame.cpp:533
#47 0xb56059d9 in nsHTMLScrollFrame::ReflowContents (this=0xae9dd190, aState=0xbf97cb24, aDesiredSize=...)
    at /home/alon/Dev/mozilla-central/layout/generic/nsGfxScrollFrame.cpp:625
#48 0xb56066b1 in nsHTMLScrollFrame::Reflow (this=0xae9dd190, aPresContext=0xae9c1c00, aDesiredSize=..., aReflowState=..., 
    aStatus=@0xbf97d0d4) at /home/alon/Dev/mozilla-central/layout/generic/nsGfxScrollFrame.cpp:866
#49 0xb55d9544 in nsContainerFrame::ReflowChild (this=0xae9db310, aKidFrame=0xae9dd190, aPresContext=0xae9c1c00, aDesiredSize=..., 
    aReflowState=..., aX=0, aY=0, aFlags=0, aStatus=@0xbf97d0d4, aTracker=0x0)
    at /home/alon/Dev/mozilla-central/layout/generic/nsContainerFrame.cpp:740
#50 0xb5686b1a in ViewportFrame::Reflow (this=0xae9db310, aPresContext=0xae9c1c00, aDesiredSize=..., aReflowState=..., aStatus=@0xbf97d0d4)
    at /home/alon/Dev/mozilla-central/layout/generic/nsViewportFrame.cpp:293
#51 0xb5573541 in PresShell::DoReflow (this=0xb3b88780, target=0xae9db310, aInterruptible=1)
    at /home/alon/Dev/mozilla-central/layout/base/nsPresShell.cpp:7843
#52 0xb5573cb0 in PresShell::ProcessReflowCommands (this=0xb3b88780, aInterruptible=1)
    at /home/alon/Dev/mozilla-central/layout/base/nsPresShell.cpp:7982
#53 0xb556a57d in PresShell::FlushPendingNotifications (this=0xb3b88780, aType=Flush_InterruptibleLayout)
    at /home/alon/Dev/mozilla-central/layout/base/nsPresShell.cpp:4897
#54 0xb5582217 in nsRefreshDriver::Notify (this=0xaeaca500) at /home/alon/Dev/mozilla-central/layout/base/nsRefreshDriver.cpp:326


> > The question I have is why the |if (!mAttachedToParent)| check is there 
> 
> Because if mAttachedToParent, we don't own the widget and shouldn't be messing
> with its visibility, last I checked.
> [..]
> Because the widget's visibility is controlled by whoever owns the widget.  In
> the Windows case before your patch, the attached case only happens for the
> toplevel window, and its visibility is handled by nsXULWindow and the like.  It
> shouldn't be affected by the xul loaded in the window.

Oh, ok, now I am starting to understand the logic here. That makes sense.

So the question is, why is Show(false) called for puppet widgets, that is, why does that reflow mentioned above end up calling it, and why *doesn't* this happen in Windows. (I don't know much about the reflow code, but looking along that trace, nothing odd pops out - it seems to be normal behavior.)
Attachment #508611 - Attachment is obsolete: true
> Well, the member variable is actually used as 'now attached'. 

Except in MakeWindow.  And with your changes in Show....  That's a lot of exceptions, compared to number of uses.  ;)

> It's hidden from a reflow

Uh... that's a combobox hiding its dropdown (which has its own widget, last I checked).  If _that_ is hiding the puppet widget, sounds like there's some serious bugginess going on there.  Unless this is a different puppet widget from the one the page is attaching to!
(In reply to comment #88)
> Uh... that's a combobox hiding its dropdown (which has its own widget, last I
> checked).  If _that_ is hiding the puppet widget, sounds like there's some
> serious bugginess going on there.  Unless this is a different puppet widget
> from the one the page is attaching to!

Fennec doesn't do comboboxes the same way as Firefox. I thought they didn't use widgets for that? Seems like there is a problem here that needs looking into.
(In reply to comment #88)
> > Well, the member variable is actually used as 'now attached'. 
> 
> Except in MakeWindow.  And with your changes in Show....  That's a lot of
> exceptions, compared to number of uses.  ;)
> 

I don't understand about ::Show. My changes there are consistent with mIsAttached meaning 'is currently attached'. The only issue is with MakeWindow as far as I can see (and that should be ok now, in the latest patch). Or am I missing something?

> > It's hidden from a reflow
> 
> Uh... that's a combobox hiding its dropdown (which has its own widget, last I
> checked).  If _that_ is hiding the puppet widget, sounds like there's some
> serious bugginess going on there.  Unless this is a different puppet widget
> from the one the page is attaching to!
(In reply to comment #88)
> Fennec doesn't do comboboxes the same way as Firefox. I thought they didn't use
> widgets for that? Seems like there is a problem here that needs looking into.

You guys are right, something bad is happening here. stechz confirms that Fennec should only have comboboxes etc. rendered in frontend code in the parent process, and not platform code in the child process as we are encountering here. So something is very very wrong. I'll find out if it happens without this patch as well.
Depends on: 631049
Attached patch patch (obsolete) — Splinter Review
The combobox issue happens without this patch, so it's a separate matter. I filed bug 631049, and updated this patch with a comment about the call to Show() being a temporary workaround for that bug.

I hope that answers all the open questions about this patch, or are there remaining issues?
Attachment #509203 - Attachment is obsolete: true
(In reply to comment #91)
> The combobox issue happens without this patch, so it's a separate matter. I
> filed bug 631049, and updated this patch with a comment about the call to
> Show() being a temporary workaround for that bug.

The call in bug 631049 looks to be a call on a widget created for the combobox's dropdown, not the top level puppet widget. Have you verified that the widgets are the same for those calls?
I didn't match them up during the same run, but

1) The page widget we care about does get hidden. Without a call to show, it will not appear.
2) The only call to hide a widget is the combobox stack I showed.

so I assume they are connected.

One guess, is that the combobox ends up reusing the same widget we are reusing for the pages somehow. This would explain all of the above. Or maybe showing the combobox widget leads to hiding the one we care about if it is defined improperly somehow (perhaps through the view). But I think it is a moot point anyhow since the combobox shouldn't exist at all.
(In reply to comment #93)
> One guess, is that the combobox ends up reusing the same widget we are reusing
> for the pages somehow. This would explain all of the above. Or maybe showing
> the combobox widget leads to hiding the one we care about if it is defined
> improperly somehow (perhaps through the view). But I think it is a moot point
> anyhow since the combobox shouldn't exist at all.

If any of the things you list here are happening at all then that is a serious problem period. If we are just creating an unused puppet widget for the combobox dropdown then that isn't as big a deal.
> I don't understand about ::Show.

Nevermind; I'm misremembering the code.

> a temporary workaround for that bug.

I don't think we should be landing such a temporary workaround.  It'll come back and bite us, badly, is my gut feeling.  Sprinkling magic show calls like that means that at some point you'll show something that should actually be hidden...

If this bug depends on another bug, fine. But then we should fix that other bug.
(In reply to comment #94)
> If any of the things you list here are happening at all then that is a serious
> problem period. If we are just creating an unused puppet widget for the
> combobox dropdown then that isn't as big a deal.

Ok, further investigation shows that these two traces occur, when clicking to browse to a new page:

#0  mozilla::widget::PuppetWidget::Show (this=0xaeb18860, aState=1)
    at /home/alon/Dev/mozilla-central/widget/src/xpwidgets/PuppetWidget.cpp:171
#1  0xb5b89498 in nsView::NotifyEffectiveVisibilityChanged (this=0xaf62cd60, aEffectivelyVisible=1)
    at /home/alon/Dev/mozilla-central/view/src/nsView.cpp:510
#2  0xb5b89523 in nsView::SetVisibility (this=0xaf62cd60, aVisibility=nsViewVisibility_kShow)
    at /home/alon/Dev/mozilla-central/view/src/nsView.cpp:529
#3  0xb5b8a1cb in nsView::InitializeWindow (this=0xaf62cd60, aEnableDragDrop=1, aResetVisibility=1)
    at /home/alon/Dev/mozilla-central/view/src/nsView.cpp:837
#4  0xb5b8a091 in nsView::CreateWidgetForPopup (this=0xaf62cd60, aWidgetInitData=0xbfe63830, aParentWidget=0x0, aEnableDragDrop=1, 
    aResetVisibility=1) at /home/alon/Dev/mozilla-central/view/src/nsView.cpp:813
#5  0xb5b899fb in nsIView::CreateWidgetForPopup (this=0xaf62cd60, aWidgetInitData=0xbfe63830, aParentWidget=0x0, aEnableDragDrop=1, 
    aResetVisibility=1) at /home/alon/Dev/mozilla-central/view/src/nsView.cpp:685

and then

#0  mozilla::widget::PuppetWidget::Show (this=0xaeb18860, aState=0)
    at /home/alon/Dev/mozilla-central/widget/src/xpwidgets/PuppetWidget.cpp:171
#1  0xb5b894ba in nsView::NotifyEffectiveVisibilityChanged (this=0xaf62cd60, aEffectivelyVisible=0)
    at /home/alon/Dev/mozilla-central/view/src/nsView.cpp:513
#2  0xb5b89523 in nsView::SetVisibility (this=0xaf62cd60, aVisibility=nsViewVisibility_kHide)
    at /home/alon/Dev/mozilla-central/view/src/nsView.cpp:529
#3  0xb5b8fc2e in nsViewManager::SetViewVisibility (this=0xaeb07830, aView=0xaf62cd60, aVisible=nsViewVisibility_kHide)
    at /home/alon/Dev/mozilla-central/view/src/nsViewManager.cpp:1398
#4  0xb55ad2e6 in nsComboboxControlFrame::ReflowDropdown (this=0xb02baa90, aPresContext=0xb22e1400, aReflowState=...)
    at /home/alon/Dev/mozilla-central/layout/forms/nsComboboxControlFrame.cpp:455
#5  0xb55ad9fd in nsComboboxControlFrame::Reflow (this=0xb02baa90, aPresContext=0xb22e1400, aDesiredSize=..., aReflowState=..., 
    aStatus=@0xbfe5f1a8) at /home/alon/Dev/mozilla-central/layout/forms/nsComboboxControlFrame.cpp:643

So the important puppet widget is shown normally (the same one appears in both traces), and is then hidden due to the combobox.

Note that the view is the same as well. This seems to be important, since in both stacks, we get to the widget through the view's mWindow (so, having the same view leads to working on the same widget). Should the combobox and document have the same view?
> Should the combobox and document have the same view?

Normally, no.
OK, those stacks look like they're for the popup widget.  So the view being equal there is fine.  And that shouldn't be the same widget as the page toplevel widget that the document viewer has a pointer to, according to both my code-inspection and cjones's description of the code's intent.  Can you check whether the widget being shown/hidden there is the same as the widget the document viewer has?
Duplicate of this bug: 631450
Attached patch patchSplinter Review
Well, I updated m-c this morning, and rebuilt... and it works without the additional hack in |Show|. Very odd...

I can bisect to find what caused the positive change. But perhaps the patch can be reviewed meanwhile?

Attached is the patch without the |Show| hack.
Attachment #509269 - Attachment is obsolete: true
I cannot reproduce the problem on older builds. There must have been something wrong with my build before, I can't explain it any other way.

In any case, the attached patch should be ready for review, unless there are other open issues? If not, who would like to review this?
I'm sure Boris doesn't *want* to review it, but he probably should.
Comment on attachment 509959 [details] [diff] [review]
patch

Ok, asking bz.

Pushed to try meanwhile to make sure this has not broken anything on Windows (which also has shared widgets).
Attachment #509959 - Flags: review?(bzbarsky)
Summary: Cannot enter text after pressing the system Back button → Reuse a single puppet widget
Comment on attachment 509959 [details] [diff] [review]
patch

>+++ b/layout/base/nsDocumentViewer.cpp
>@@ -1387,16 +1392,42 @@ DocumentViewerImpl::Open(nsISupports *aS
>+  // When loading a page from the bfcache with puppet widgets

So....  Is there a reason this needs to only happen for puppet widgets?  What would go wrong if we just did this if mPresContext && ShouldAttachToTopLevel()?  To keep risk down, I'm ok with answering this question in a followup post-2.0 bug.

Also, shouldn't this really test mViewManager?  That's what you care about...

>+    NS_ABORT_IF_FALSE(!!v, "no root view");
>+    NS_ABORT_IF_FALSE(!!mParentWidget, "no mParentWidget to set");

You don't need the !! in either of those lines.

>@@ -1928,19 +1959,18 @@ DocumentViewerImpl::Show(void)
>-    if (!mAttachedToParent) {
>+    if (!mAttachedToParent)
>       mWindow->Show(PR_TRUE);
>-    }

Please leave the curlies.

> DocumentViewerImpl::MakeWindow(const nsSize& aSize, nsIView* aContainerView)

>+  PRBool shouldAttach = PR_FALSE;
>+
>+  if (ShouldAttachToTopLevel()) {

Just init shouldAttach = ShouldAttachToTopLevel(), and then test shouldAttach in the if condition (and no need to set it in the if body).

r=me with the nits above on the layout bits; I'mm assuming the puppet widget changes are correct.
Attachment #509959 - Flags: review?(bzbarsky) → review+
Comment on attachment 509959 [details] [diff] [review]
patch

Thanks bz, I will make those changes.

cjones, can you review the PuppetWidget parts?

Looks good on try (still waiting for a few final runs though).
Attachment #509959 - Flags: review?(jones.chris.g)
Comment on attachment 509959 [details] [diff] [review]
patch

>diff --git a/widget/src/xpwidgets/PuppetWidget.cpp b/widget/src/xpwidgets/PuppetWidget.cpp
>--- a/widget/src/xpwidgets/PuppetWidget.cpp
>+++ b/widget/src/xpwidgets/PuppetWidget.cpp
>@@ -272,45 +272,43 @@ NS_IMETHODIMP
> PuppetWidget::DispatchEvent(nsGUIEvent* event, nsEventStatus& aStatus)
> {

Also assert here !mChild || mChild->mWindowType == eWindowType_popup.

It would be nice to kill the mChild hack entirely, but that adds risk wrt the phony popups we create.  I'm fine with fixing that later.
Attachment #509959 - Flags: review?(jones.chris.g) → review+
Blocks: 632248
> >+++ b/layout/base/nsDocumentViewer.cpp
> >@@ -1387,16 +1392,42 @@ DocumentViewerImpl::Open(nsISupports *aS
> >+  // When loading a page from the bfcache with puppet widgets
> 
> So....  Is there a reason this needs to only happen for puppet widgets?  What
> would go wrong if we just did this if mPresContext && ShouldAttachToTopLevel()?
>  To keep risk down, I'm ok with answering this question in a followup post-2.0
> bug.
> 

Filed followup bug 632248 for this.
No longer depends on: 631049
http://hg.mozilla.org/mozilla-central/rev/e71960b6957c
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
VERIFIED FIXED on:
Build Id:
Mozilla /5.0 (Android;Linux armv7l;rv:2.0b12pre) Gecko/20110210
Firefox/4.0b12pre Fennec /4.0b5pre 

Device: Motorola Droid 2 (Android 2.2)
Status: RESOLVED → VERIFIED
Depends on: 637187
https://litmus.mozilla.org/show_test.cgi?id=15213
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.