132175 - carve out datasource from window mediator

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P1)

Description

[Alec Flett]

As a part of bug 107059, we're trying to remove RDF requirements from embedding
components. right now nsWindowMediator uses RDF as its storage mechanism... 

This will be a little tricky, because nsWindowMediator implements
nsIRDFDataSource, in order to provide things like a task list, and so forth..in
addition, it has an inner member, mInner, which is an in-memory datasource, and
is the actual in-memory storage mechanism for the windows lists.

I'd like to continue to share some implementation with seamonkey. My guess is
that I'll break apart the existing implementation into a core list of
windows/titles/etc, which will be for embedding-only, then I'll implement a new
datasource that translates the RDF calls into calls into the window mediator.
This will also involve some kind of notification mechanism so that the window
mediator can notify the datasource when things like window titles have changed.

Comment 1

[Alec Flett]

ugh. this is going to suck.

I've looked through this more, and trying to duplicate the window mediator would
be really annoying, and trying to maintain two mediator implementations would be
a lot harder than I thought. I was under the impression that the mediator just
did some basic window list type things, but it also handles zorder and even a
little modal window crap.

I think what we need instead is two things:
1) the window mediator should not use RDF for internal storage
2) there should be a seperate windowlist datasource, which watches the existing
window mediator for updates. We can either
  - try to do all the notifications via nsIObserver
  - invent a new interface to indicate window changes

I'm leaning towards making a new interface.

Here's what the datasource is used for:
- UpdateWindowTitle() - to Unassert the old title, and Assert the new one
- AddWindowToRDF() - called by RegisterWindow, to add a new window
- RemoveAndUpdateSynthetics() - called by UnregisterWindow, to remove a window,
and update any keyindexes

And thus I'm going suggest a new interface:

interface nsIWindowMediatorListener : public nsISupports {
  void onTitleChange(in nsIXULWindow window, in string title);
  void onNewWindow(in nsIXULWindow);
  void onWindowRemoved(in nsIXUlWindow);
};

and the datasource will implement this, and do the right glue to update its
datasource.

Other issues that will make this tricky:
- nsWindowInfo contains the RDF name (which is window-<timestamp>) of the window
(in mRDFID) so now the datasource will have to maintain a list of window->rdfid
associations. This might not be so bad.
- splitting this into 2 objects where only one implements nsIRDFDataSource might
make it tricky for callers. From what I can tell, MOST callers just want the
nsIWindowMediator interface.
- where will the datasource live now? Somewhere in xpfe/components I think.
- hopefully there is no other meta-data in nsWindowInfo that the datasource
needs other than mRDFID, and hopefully there is no data stored in the datasource
that the window mediator will need.

Comment 2

[Alec Flett]

ok I'm close to finished - the one thing I've run into with this is
getWindowForResource. Basically at the XUL level we need some way to
1) determine if a resource-id'd window is focused so we can set the checkbox
2) find a window based on its resource to raise it if someone selects it from
the "window" menu

My initial thought is just to tack this onto nsIWindowMediatorListener or
something. However, it be a seperate interface that's specific to the window
data source, to keep RDF semantics out of the nsIWindowMediator* interfaces. I
might even make an nsIWindowDataSource or something. Yes. That's what I'll do. joy.

Comment 3

[Alec Flett]

Attachment: rip rdf out of window mediator

so this patch should supposedly be the fix, but I am getting the STRANGEST
crash with it - basically I'm crashing somewhere in what appears to be
nsDocShell's vtable, which makes no sense to me. I even ran this under purify
to see if I was dealing with a Release()'ed object or something, but no luck
there either. My guess is there are some funky ownership issues with these
window objects that I'm just not understanding. 

Anyway, I'm basically attaching here so I can track my own progress in nailing
this down.

Comment 4

[Alec Flett]

augh! this is killing me. Ok, my latest discovery is that again, with this patch
someone is whacking nsXULWindow's vtable for nsIXULWindow - I don't get it at
all. Continuing to investigate.

Comment 5

[Alec Flett]

ah! duh! I figured out the problem. it was just stupidity on my part.. new patch
coming up. Basically I was calling notifyTitleChange(inWindow,...) when I should
have been calling mListeners->EnumerateForwards(notifyTitleChange, ...)

so I ended up passing in the current nsIXULWindow to notifyTitleChange, which
ended up calling into some random entry in nsIDocShell's vtable, which ended up
overwriting the vtable.

Comment 6

[Alec Flett]

Attachment: remove RDF, clean up inheritance, add windowds to build

Ok, this patch
1) fixes the bug - removes RDF support from the window data source
2) fixes wierd inheritance issues amongst nsWebShellWindow/nsXULWindow/etc -
previously the inherited class had two versions of QI, and the child class
wouldn't pass its QI results up to the parent, which resulted in all sorts of
COM QI contract violations - not to mention two different mRefCnts for the same
class. this also meant I had to make the base class be threadsafe, but that
shouldn't hurt anyone.
3) adds the window data source to the build. You can see the
currently-not-part-of-build directory here:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/windowds/
4) various other cleanups, as you'll see in the patch (including some removal
of some WAY old #if 0'ed code regarding menu creation! What was that doing in
there? )

Comment 7

[Alec Flett]

Attachment: update JS consumers as well

this is in addition to the above patch - there are only two consumers here, and
this makes sure the Window (tasks) menu continues to work.

Comment 8

[Alec Flett]

Attachment: update JS consumers as well v1.1

oops, somehow missed the inspector in that diff

Comment 9

[Alec Flett]

ok, these patches are now ready for review. I've tested them on windows, mac,
and linux. 

Again, heres what needs review:
http://bugzilla.mozilla.org/attachment.cgi?id=87745&action=edit
http://bugzilla.mozilla.org/attachment.cgi?id=88033&action=edit
http://lxr.mozilla.org/seamonkey/source/xpfe/components/windowds/ (already
checked in code, not part of build until the other patches land)

oh, and you can ignore that printf about "AddListener(...)" - I've removed that
from my tree.

Comment 10

[Alec Flett]

adding waterson/bnesse onto the CC for sr/r. This bug explains the patch pretty
well (its mostly me talking to myself anyway) but let me know if you have
questions...thanks guys.

Comment 11

[Brian Nesse (gone)]

Comment on attachment 87745 [details] [diff] [review]
remove RDF, clean up inheritance, add windowds to build

You've added at least 1 idl file... I don't see any Mac build foo here...

 NS_IMETHODIMP nsWindowMediator::RegisterWindow(nsIXULWindow* inWindow)
 {
-  if (inWindow == NULL)
-    return NS_ERROR_INVALID_ARG;
-  
   mTimeStamp++;
Was there a reason you removed this?

Comment 12

[Brian Nesse (gone)]

Comment on attachment 88033 [details] [diff] [review]
update JS consumers as well v1.1

Looks pretty straightforward. r=bnesse.

Comment 13

[Alec Flett]

oh, yeah the mac stuff is just adding those files to the build - I had attached
these patches from my pc, then tested them on the mac and did the project work
over there - can we just imagine that I'm adding the .idl to the mac build?  :)
I'm also adding nsWindowDataSource (See the lxr link) to mozcomps and
nsIWindowDataSource to appcompsIDL

Comment 14

[Brian Nesse (gone)]

Comment on attachment 87745 [details] [diff] [review]
remove RDF, clean up inheritance, add windowds to build

Fine with me... as long as we don't just image that we checked it in too. ;)
r=bnesse.

Comment 15

[Chris McAfee]

Comment on attachment 87745 [details] [diff] [review]
remove RDF, clean up inheritance, add windowds to build

appshell/src/nsWebShellWindow.cpp: 
removed #if 0 code:
  void nsWebShellWindow::LoadMenus()
  void nsWebShellWindow::LoadSubMenu()
corresponding #if 0 code should be removed from
appshell/src/nsWebShellWindow.h

r=mcafee, please test and make sure this works :-)

Comment 16

[Chris Waterson]

Comment on attachment 87745 [details] [diff] [review]
remove RDF, clean up inheritance, add windowds to build

rs=waterson

Comment 17

[Chris Waterson]

Comment on attachment 88033 [details] [diff] [review]
update JS consumers as well v1.1

rs=waterson

Comment 18

[Alec Flett]

ok, with a few huccups, this is fixed.

Comment 19

[Ere Maijala (slow)]

It seems to me that this patch has caused bug 155157.

Comment 20

[Jean-Francois Ducarroz]

oops, there is still code that use the old contactid for the window-mediator out
there! in both Mozilla and Netscape tree!!! see also bug 156876

Comment 21

[Shanmugavelu Shanmuganathan (gone)]

I see a crash in the trunk build recently on a Tru64 UNIX, a 64-bit OS. 
I see the behaviour described on comment #3.

Here are the debug informations.
Thread received signal SEGV
stopped at [virtual nsresult nsWindowDataSource::GetWindowForResource(const
char*, class nsIDOMWindowInternal**):366 0x300091e51f0]
    366         closure.resultWindow->GetDocShell(getter_AddRefs(docShell));
(ladebug) p docShell
class nsCOMPtr<nsIDocShell> { 
  mRawPtr = 0x0;                  // class nsCOMPtr_base
}
(ladebug) t 5
>0  0x300091e51f0 in
((nsWindowDataSource*)0x14094de00)->nsWindowDataSource::GetWindowForResource(aResourceString=0x140c66580="window-0",
aResult=0x11fff8578) "nsWindowDataSource.cpp":366
#1  0x3ffbfef6f9c in XPTC_InvokeByIndex() "xptcinvoke_asm_osf1_alpha.s":73
#2  0x3ffbfccdc5c in XPCWrappedNative::CallMethod(ccx=& class XPCCallContext {
... }, mode=CALL_SETTER) "xpcwrappednative.cpp":1993
#3  0x3ffbfcdd8ec in XPC_WN_CallMethod(cx=0x1400e0300, obj=0x140ae86b0, argc=1,
argv=0x0, vp=0x11fff8838) "xpcwrappednativejsops.cpp":1266
#4  0x3ffbff81060 in js_Invoke(cx=0x1400e0300, argc=1, flags=0) "jsinterp.c":788

Comment 22

[Shanmugavelu Shanmuganathan (gone)]

The crash I reported here happens as soon as I click the "Window" 
menu.

Comment 23

[Alec Flett]

well that's odd then.. the crash in comment 3 was definitely fixed before the
patch landed - it was an improper cast from one object to another. the stack you
posted looks like a null-pointer dereference though...

Comment 24

[Shanmugavelu Shanmuganathan (gone)]

I know what the bug is. 
The checkin through revision 1.7  to nsWindowDataSource.cpp changes 
code which treats pointers to be 32 bits, which causes trouble on a 64 bit OS.

One such example is  
nsISupportsKey key(window);   has been changed to                              
               

nsPRUint32Key key(NS_PTR_TO_INT32(window)); 

I hit the bug when closure->resultWindow   is stuffed with 32 bits
                   
closure->resultWindow = NS_STATIC_CAST(nsIXULWindow*,
NS_INT32_TO_PTR(thisKey->GetValue()));

I suggest this to be changed to work on 64 bit OS.

Comment 25

[timeless]

reopening

Comment 26

[Alec Flett]

reclosing. file a new bug. The bug, "carve out datasource from window mediator"
has been fixed. It just happened to introduce a NEW bug on 64-bit machines.

Comment 27

[Michael Dunn]

Clean up verification of dated code change bus