80946 - Cannot dismiss Pref dialog by "OK" after visiting "Advanced -> System" category

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect, P3)

Description

[doctor__j]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9+) Gecko/20010515
BuildID:    2001051504

Reproducible: Always
Steps to Reproduce:
1. Open Pref menu
2. Choose "Advanced -> System" category
3. Press OK button to dismiss Pref menu

Actual Results:  Cannot close Pref menu by OK button.  Pref menu can be closed
by Cancel button.

Expected Results:  Dismiss Pref menu by OK button.

I saw this error msg from the JavaScript Console:

Error: pageData has no properties
Source File: chrome://communicator/content/pref/nsPrefWindow.js
Line: 207

Comment 1

[Chris Lyon]

I'm seeing this bug too on 2001051504 on Win2K

Comment 2

[[e-mail]]

Verified on 2001051504 Win32 Talkback, marking as NEW

Moz Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9+) Gecko/20010515

Comment 3

[sairuh (rarely reading bugmail)]

ouch, regression.

ben or blake, would this be yours?

Comment 4

[Blake Ross]

argh.  so...many...regressions...mcafee or eddyk, could you take a look at this?

Comment 5

[Stephen Walker]

*** Bug 81813 has been marked as a duplicate of this bug. ***

Comment 6

[eddyk]

So far, I see that we're getting some empty variables back.  But I'm not seeing 
why.  A quick hack (just for illustration, I don't think this is the right 
approach yet) works around the problem.

Ben, any idea why or how wsm might not return pageData?  Or put another way, 
what is required to setup a page so that pageData is properly initialised?

An interesting observation about this panel is that is makes no use of any pref 
attributes.  Everything is setup and saved using js to the registry.  Would not 
using any pref attributes have anything to do with this?

+++ nsPrefWindow.js     2001/05/29 22:15:32
@@ -74,7 +74,7 @@

       init:
         function ()
-          {
+          {
             if( window.queuedTag )
               {
                 this.onpageload( window.queuedTag );
@@ -204,6 +204,9 @@
             for( var pageTag in this.wsm.dataManager.pageData )
               {
                 var pageData = this.wsm.dataManager.getPageData( pageTag );
+if (!pageData) continue;

Comment 7

[Skewer]

*** Bug 83861 has been marked as a duplicate of this bug. ***

Comment 8

[Hixie (not reading bugmail)]

This causes data loss because any data entered in the dialog on some other panes
is subsequently lost if you can't save it by clicking OK...

Comment 9

[doctor__j]

Hixie: If you press "OK" button first, before you press the "Cancel" button, the
modification to other pref items *will* be applied and saved.  It's just the
illusion that you can't commit those pref changes.  Have a try.

Comment 10

[eddyk]

I've backed out the changes I made for the pref locking in nsPrefWindow and the 
error still occurs.  Per jpm I'm assigning to vishy.

Comment 11

[Hixie (not reading bugmail)]

doctor__j: ok, now that's some screwed up stuff right there. ;-)

Comment 12

[Paul Chen]

nav triage team:

marking p3

Comment 13

[Paul Chen]

nav triage team:

Forgot to add nsbeta1+

Comment 14

[R.K.Aa.]

*** Bug 84635 has been marked as a duplicate of this bug. ***

Comment 15

[doctor__j]

*** Bug 84730 has been marked as a duplicate of this bug. ***

Comment 16

[R.K.Aa.]

*** Bug 85099 has been marked as a duplicate of this bug. ***

Comment 17

[sairuh (rarely reading bugmail)]

*** Bug 83884 has been marked as a duplicate of this bug. ***

Comment 18

[Fabian Guisset]

*** Bug 85174 has been marked as a duplicate of this bug. ***

Comment 19

[Emlyn Shannon]

bug 79821 is similar. Might be worth a look.

Comment 20

[Chris Lyon]

*** Bug 85337 has been marked as a duplicate of this bug. ***

Comment 21

[Keyser Sose]

*** Bug 85004 has been marked as a duplicate of this bug. ***

Comment 22

[doctor__j]

*** Bug 85392 has been marked as a duplicate of this bug. ***

Comment 23

[Skewer]

mostfreq added @ 10 dups

Comment 24

[Scott MacGregor]

vishy, would it be possible to get this re-assigned to an engineer? I'm pretty
sure you weren't planning on fixing it yourself =). Thanks!

Comment 25

[R.K.Aa.]

*** Bug 85778 has been marked as a duplicate of this bug. ***

Comment 26

[Viswanath Ramachandran]

I think mscott is right :-). 

Comment 27

[alanjstr]

Is this related to bug 66118?  

Comment 28

[Blake Ross]

I'll look into this.

Comment 29

[Blake Ross]

This was indirectly caused by jbetak's 5/14 checkin for bug 57720.  The problem 
was that pref-winhooks.js handles all of the pref retrieval and setting 
internally, but must use a function called GetFields() to save state upon 
switching panels, but GetFields() is also used when OK is pressed:

            hPrefWindow.wsm.savePageData( tag );
            hPrefWindow.savePrefs();

In savePageData:

          if( 'GetFields' in this.contentArea)
            {
              // save page data based on user supplied function in content area
              var dataObject = this.contentArea.GetFields();
              this.dataManager.setPageData( aPageTag, dataObject );
            }

But the GetFields() in pref-winhooks.js doesn't return an object, or anything, 
since we don't want the nsPrefWindow to save the prefs for us; we're doing it 
ourselves.  So we ended up setting the pageData to undefined, which caused this 
error.

I think handling some of the pref setting and retrieval yourself, as this panel 
does, is a valid usage.  So my morning adventures through 
nsWidgetStateManager/nsPrefWindow/pref-winhooks.* are concluded with a null 
check.

Comment 30

[Blake Ross]

Attachment: patch

Comment 31

[Blake Ross]

cc'ing alec for sr.

Comment 32

[[e-mail] (away - not reading bugmail)]

r=jbetak, thanks for pointing this out!

I probably should have done a null check in the in nsPrefWindow,
like "if (pageData && pageData.initialized)" or something similar. My 
apologies. The suggested solution is better and saves some cycles instead of 
adding more...

Comment 33

[Alec Flett]

yay!
However, have you figured out which part of the advanced system page causes
this? I'm glad to see this fix because we've hit this before, but I think it
would be useful to have a warning printed to the screen to make future broken
panels easier to debug.

Comment 34

[Blake Ross]

Yeah, we keep hitting this same end result (see bug 40891 for all the fun 
cases), but nearly all of them are separate cases unfortunately.  This one was 
also pretty isolated.  The problem in this file was:

function GetFields() {
    // Get globals.
    var settings = parent.winHooks.settings;
    var winhooks = parent.winHooks.winhooks;
    var prefs    = parent.winHooks.prefs;
    // Transfer data from dialog to prefs object.
    for( var index in settings ) {
        var setting = settings[ index ];
        var checkbox = document.getElementById( setting );
        if ( checkbox ) {
            prefs[ setting ] = checkbox.checked;
        }
    }
}

GetFields() is used in two places -- to temporarily persist state when 
switching between pref panels, and to save prefs (that the function returns 
in an array) when the OK button is pressed.  nsWidgetStateManager assumes that 
every client needs both usages.  In this case, winhooks is doing all of the 
setting, retrieving and saving itself (because it really retrieves the info from 
the registry, not prefs), so there's no need for 
nsPrefWindow/nsWidgetStateManager to save anything upon pressing OK, and thus 
it doesn't return an array.  I don't think we really need a warning for this, 
since future panels that do this should no longer break with this fix.
 

Comment 35

[Keyser Sose]

*** Bug 86057 has been marked as a duplicate of this bug. ***

Comment 36

[Scott MacGregor]

this needs to be checked in for rtm, I don't know why it's triaged out to .9.3.
That's too late! Blake, any chance you can get this checked in b4 midnight
tomorrow? 

sr=mscott on your fix. I applied the patch and it fixes it for me. 

Comment 37

[Blake Ross]

Sorry, forgot to bring this in.  Yeah, I'll get approval and check it in 
tonight.

Comment 38

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 39

[Blake Ross]

Fix checked in.

Comment 40

[Chris Lyon]

*** Bug 86815 has been marked as a duplicate of this bug. ***

Comment 41

[Chad Austin]

verified win2k 2001062004

Comment 42

[Keyser Sose]

*** Bug 88370 has been marked as a duplicate of this bug. ***

Comment 43

[Chris Lyon]

*** Bug 90600 has been marked as a duplicate of this bug. ***

Comment 44

[Thomas Guyot]

This bug has reapeared in build 2001071214 see the latest dupe (bug 90600)

Comment 45

[Thomas Guyot]

Not dupe anymore - sorry for spam