129288 - Changing font size pref in PPEmbed crashes

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Conrad Carlen (not reading bugmail)]

This was noticed in testing bug 128924. If you change the font size pref and
click OK, you'll crash with this stack (the cream was skimmed off the top):

 #0   0x0076befc in nsQueryInterface::_cl( const(nsID const &, void **))
 #1   0x0076bf04 in nsQueryInterface::_cl( const(nsID const &, void **))
 #2   0x0323f70c in assign_from_helper__28nsCOMPtr<16nsIPopupSetFrame>FRC15nsCOMPt
 #3   0x03216f48 in ConstructXULFrame__21nsCSSFrameConstructorFP12nsIPresShellP14n
 #4   0x0321b15c in ConstructFrameInternal__21nsCSSFrameConstructorFP12nsIPresShel
 #5   0x0321a7b0 in ConstructFrame__21nsCSSFrameConstructorFP12nsIPresShellP14nsIP
 #6   0x03214f60 in nsCSSFrameConstructor::CreateAnonymousFrames(nsIPresShell *)
 #7   0x0321477c in nsCSSFrameConstructor::CreateAnonymousFrames(nsIPresShell *)
 #8   0x0320e60c in ConstructDocElementFrame__21nsCSSFrameConstructorFP12nsIPresSh
 #9   0x0321bbf0 in ReconstructDocElementHierarchy__21nsCSSFrameConstructorFP14nsI
 #10  0x0202e66c in ReconstructDocElementHierarchy__12StyleSetImplFP14nsIPresConte
 #11  0x030baf38 in PresShell::ReconstructFrames(void)
 #12  0x030a5384 in PresShell::SetPreferenceStyleRules(int)
 #13  0x03094da8 in nsPresContext::PreferenceChanged(char const *)
 #14  0x03090898 in nsPresContext::PrefChangedCallback(char const *, void *)
 #15  0x01bf7664 in pref_DoCallback(char const *)
 #16  0x01bf6f1c in pref_HashPref(char const *, PrefValue, PrefType, PrefAction)
 #17  0x01bf5540 in PREF_SetIntPref

As expected, the change in font size causes the frames to be reconstructed. Why
there is a difference in an embedded context and seamonkey in this respect is
puzzling.

Comment 1

[Conrad Carlen (not reading bugmail)]

I've got a fix for this. See this:
http://lxr.mozilla.org/mozilla/source/layout/base/src/nsPresContext.cpp#648
The top-level docshelltreeitem in the window put up by PPEmbed is not
typeChrome. Fixing that so it is chrome (in PPEmbed) fixes this.

Comment 2

[Conrad Carlen (not reading bugmail)]

Attachment: patch

Fixes this and 2 other problems with XUL pref dialog in PPEmbed. Notice that
the patch adds another const to nsIWebBrowserSetup which is frozen. I think
that, when something's frozen, its API is frozen, not the values which can be
passed to that API (which is all a constant is). Jud, what's your opinion on
this?

Comment 3

[Conrad Carlen (not reading bugmail)]

Adam, can I get r= on the patch?

Comment 4

[Conrad Carlen (not reading bugmail)]

CC'ing chak for his opinion on adding a const to a frozen iface. See comment 2.

Comment 5

[Adam Lock]

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

Looks good, r=adamlock

Comment 6

[Chak Nanga]

Hi Conrad : I don't see any problem in adding a const to a frozen interface -
especially since we're not changing any methid interfaces. Alec/Jud?

Comment 7

[Conrad Carlen (not reading bugmail)]

Alec, while you're here, can you sr=?

Comment 8

[Judson Valeski]

To be specific... adding (*not* removing which would be illegal) of consts to
frozen iface is something I believe everyone's bought off on. 

ccarlen wrote: 

"I think that, when something's frozen, its API is frozen, not the values which
can be passed to that API (which is all a constant is)."

"values" is too general in your stmt IMO. change that to "const values" and I
think we're on the same page.

Comment 9

[Alec Flett]

hmm... the changing/addition of frozen interface constants has been discussed
before.. as I recall, people were against it. I'm on the fence, but I'm going to
lean towards it being ok. Here is my argument:

By making APIs which depend on numeric values for their behavior, we're already
making the interface more dynamic. In this case, we have setProperty() which
takes an arbitrary ID as its parameter. If we didn't want to be dynamic, we
should have done something such as having seperate "long" attributes for each
property, such as 

attribute long allowPlugins;

However, I'm not sure I'm cool with changing the interface itself to add the
constant. I think its time for a new interface... the problem is if that
interface needs to be frozen for any reason. (i.e. if embeddors need
SETUP_IS_CHROME_WRAPPEr) If the interface does NOT need to be frozen, I say we
create another interface, nsIWebBrowserSetupChrome and put other chrome
attributes in there.

I'm adding dougt for his input as well..

Comment 10

[Doug Turner (:dougt)]

First, I am against adding constants to an IDL.  I think that they should just
be documented as well known values.  If we did it that way, we probably wouldnt
be talking about this now.  (don't argue namespace! ns!@#$)

A couple of problems when we add a new constant to a interface.  First, the
typelib for that interface is going to be different.  Secondly, will cause a
rebuild of any thing that is build against this header.  Both of which can be
argued are minor.  But both should not happen with frozen interface.

Lastly, you should add a @since xxx javadoc tag to the IDL indicating that some
code that implements this interface may not expect that new constant.  Usually
in the case where the binary does not match the headers, ect. 

Comment 11

[Conrad Carlen (not reading bugmail)]

The alternatives to adding a constant don't look good:
* Creating a new interface (nsIWebBrowserSetupChrome) just for this value is
overkill. AFAIK, toggling this flag would be it's only purpose. There aren't any
other chrome setup tasks that this new iface could do as well.
* Not adding the constant to the idl and just declaring it in the impls
everywhere would cause documentation problems.
* If we didn't actually declare the new constant in the IDL but instead just
documented it there, we'd have an icky inconsistency.

So, the best I can come up with is to add "@since mozilla1.0" to the javadoc
Alec, unless you see another way around this, can you sr= it? 


Index: mozilla/embedding/browser/webBrowser/nsIWebBrowserSetup.idl
===================================================================
RCS file: /cvsroot/mozilla/embedding/browser/webBrowser/nsIWebBrowserSetup.idl,v
retrieving revision 1.10
diff -w -u -2 -r1.10 nsIWebBrowserSetup.idl
--- mozilla/embedding/browser/webBrowser/nsIWebBrowserSetup.idl	22 Oct 2001 22:43:38 -0000	1.10
+++ mozilla/embedding/browser/webBrowser/nsIWebBrowserSetup.idl	7 Mar 2002 19:53:48 -0000
@@ -96,4 +96,14 @@
 
     /**
+     * A value of PR_TRUE makes the browser a chrome wrapper.
+     * Default is PR_FALSE.
+     *
+     * @since mozilla1.0
+     *
+     * @see setProperty
+     */
+    const unsigned long SETUP_IS_CHROME_WRAPPER     = 7;
+
+    /**
      * Sets a property on the new web browser object.
      *

Comment 12

[Conrad Carlen (not reading bugmail)]

Changing component to embedding APIs and platform to all since now I know that
the cause of this is that the embedding app cannot make the prefs window as a
chrome wrapper.

Comment 13

[Alec Flett]

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

Well, I personally prefer a new interace, but instead this can be our first
experiment with this sort of thing.

sr=alecf

Comment 14

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 15

[Conrad Carlen (not reading bugmail)]

Checked in.