99515 - Autoconfig use during migration leads to hang

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect)

Description

[lrg]

This is potentially related to http://bugzilla.mozilla.org/show_bug.cgi?id=99221

If Autoconfig is being used while migration is taking place, a hang occurs 
(though the migration process itself seems to be successful).  This forces the 
user to manually kill the netscape process.  This bug occurs 100 percent of the 
time in the case that autoconfig is being used.

Once the application is killed, it can be restarted, and the preferences that 
were migrated will be utilized, as well as those set by autoconfig (although i 
have observed some problems, as are noted in the previous bug).

Comment 1

[Mitesh Shah]

The event loop in AutoConfig is hanging after the pref-migration is done.
Looks like we have to push the current Event Queue into the current list of
eventQs so that the events posted by necko will be executed in the AutoConfig
event loop.  Patch is coming

Comment 2

[Mitesh Shah]

Attachment: Replacing GetThreadEventQueue with PushThreadEventQueue

Comment 3

[Mitesh Shah]

Looks like with the pref-migrator, the necko events are not posted to the queue 
we are getting from the GetThreadEventQueue so replacing it with 
PushThreadEventQueue. It creates a new queue and makes it a current queue. other 
code is to pop the event queue back in the error coditions. Also, moved the 
timer code after the eventloop code so it doesn't have to deal with event 
queues.

Comment 4

[Mitesh Shah]

Adding Dan for event queue code review.

Comment 5

[Dan M]

This patch's success implies the old queue (the one coming from 
GetThreadEventQueue) had already been suspended. This can happen fairly easily, 
so I guess on general principles it's probably always a good idea to push a new 
queue before falling into a captive event loop like that.

r=danm

PS you might want to consider adding a little helper class to take care of 
popping the event queue; you wouldn't have to be so careful to pop the queue at 
each possible exit point. You can crib the class at http://lxr.mozilla.org/
seamonkey/source/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#282. 
For its use (just two lines) look for "queueGuard" elsewhere in that same file.

Comment 6

[Mitesh Shah]

Comment on attachment 49361 [details] [diff] [review]
Replacing GetThreadEventQueue with PushThreadEventQueue

Putting an r= on the patch. PDT wants it r= there. see bug 97228

Comment 7

[Alec Flett]

sr=alecf

Comment 8

[Mitesh Shah]

Fix checked into the trunk. Waiting for PDT approval for branch checkin

Checking in nsAutoConfig.cpp;
/cvsroot/mozilla/modules/libpref/src/nsAutoConfig.cpp,v  <--  nsAutoConfig.cpp
new revision: 3.15; previous revision: 3.14
done

Comment 9

[grega]

lets get it in there...marking pdt+

Comment 10

[Mitesh Shah]

Fix checked into the branch.

Checking in nsAutoConfig.cpp;
/cvsroot/mozilla/modules/libpref/src/nsAutoConfig.cpp,v  <--  nsAutoConfig.cpp
new revision: 3.12.2.2; previous revision: 3.12.2.1
done                        

Comment 11

[Mitesh Shah]

Tinderbox seems fine

Comment 12

[Mitesh Shah]

Branch build on Linux seems to be hanging at the main event loop with these 
changes.

Comment 13

[Mitesh Shah]

The main event loop is hanging because even if it has stopped accepting events, 
GetThreadEventQueue return the disabled event queue when requested to return the 
youngest event queue. Now, the events posted to it get transfered to the elder 
event queue and the loop hangs waiting for the events to this queue. I think, 
GetThreadEventQueue should return the youngest active event queue. 
 
As discussed with Danm, It looks like the event queue is leaking and it should 
be deleted when it has stopped accepting events. At this point, changing 
anything in the event loop can cause regression. So I am changing AutoConfig 
event loop to use ProcessPendingEvents() instead waitforevent and handleevent. 
ProcessPendingEvents() processes the events in the elder event queues, too.

Now, another problem is that there two event queue loops, one for the AutoConfig 
and one for pref_get_ldap_attributes(). We need to create atleast one event 
queue for pref_get_ldap_attributes() because recursive calls to 
ProcessPendingEvents() on the same event queue doesn't work. 

There are two patches, one is with reference to branch version of 
nsAutoConfig.cpp and the another one is with reference to trunk version.

Comment 14

[Mitesh Shah]

Attachment: latest patch with reference to branch

Comment 15

[Mitesh Shah]

Attachment: latest patch with reference to trunk

Comment 16

[Dan M]

Mitesh, you mentioned in your comment above that you needed to push a new event 
queue in pref_get_ldap_attributes, but I don't see that in the patches. Besides 
that concern, this is the patch we've been discussing. Cool. I'm happy to 
actually understand what's happening here. Long live ProcessPendingEvents.

r=danm

again. But this time it'll all be good.

Comment 17

[Mitesh Shah]

The second event queue and the event loop is only in the patch with reference to 
the branch sources. The code is not checked into the trunk. The reason you don't 
see the PushThreadEventQueue and PopThreadEventQueue in the patch is that it's 
already in the source base as part of bug 75955 check in. 

pref_get_ldap_atribute() may be called as part of one of the events processed 
during the first event loop -> ProcessPendingEvents(). Now, this also needs to 
be finished before we can proceed further so I need another event loop to make 
it look like Sync connection. As we can see from the ProcessPendingEvents() code 
it can not be called recursivley on the same event queue so I needed to create a 
new event queue. I know it's not the best solution as we had problems with the 
main event loop hanging on the Linux earlier but with ProcessPendignEvents() it 
seems working fine so I have kept it this way. If you have any other suggestion, 
please let me know.

Thanks for your help and the code review

Comment 18

[Dan M]

That's just what event queue pushing is for, so it's possible to ProcessPending 
while in the middle of ProcessPending on some previous event. I imagine that 
pushed queue isn't leaking permanently or temporarily and causing problems like 
in the case you just fixed.

Comment 19

[Mitesh Shah]

Yes, that's right the new event queue is not leaking. It's actually getting 
deleted when pref_get_ldap_attributes() finishes. Looks like LDAP connection is 
not adding extra references like the other necko connection (mainly, HTTP) or 
may be ProcessPendingEvents() does some magic and helps to remove any references 
to the event queue.

Comment 20

[Alec Flett]

Comment on attachment 50643 [details] [diff] [review]
latest patch with reference to branch

sr=alecf

Comment 21

[Alec Flett]

Comment on attachment 50644 [details] [diff] [review]
latest patch with reference to trunk

sr=alecf

Comment 22

[Mitesh Shah]

Checked into the branch and the trunk

Comment 23

[Mitesh Shah]

Marking it fixed

Comment 24

[lrg]

I have verified this fix in the branch now, in macos8 macosX linux and windows, 
marking as verified.