Closed Bug 397753 Opened 17 years ago Closed 16 years ago

Unload code can be moved inside pagehide code in nsFormFillController.cpp

Categories

(Toolkit :: Form Manager, defect)

Product:
Toolkit
Component:
Form Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9beta4

People

(Reporter: martijn.martijn, Assigned: Dolske)

References

Details

Attachments

(1 file, 2 obsolete files)

Patch for review, v.2
8.27 KB, patch
martijn.martijn
: review+
Gavin
: review+
beltzner
: approval1.9+
Details | Diff | Splinter Review
Attached patch patch (obsolete) — — Splinter Review 
It seems to me that the unload code can be removed for a great part and a part of it moved into the pagehide code.
This avoids adding an unload listener.
Attachment #282540 - Flags: review?(dolske)
So, I'm not so sure this is correct...

The JS-pwmgr moved some old-pwmgr code into satchel to handle "pagehide". The nsIDOMLoadListener code for "unload" was already there, but wasn't modified. [I think we should have, as bug 384230 notes that this leaks as a result.]

My understanding is that "pagehide" is fired when a page moves into the fbcache by navigating away from it, and "unload" is fired when the page (tab) is simply closed. Both event handlers would thus be needed. But then, when I looked into 384230 the unload/beforeunload events didn't seem to be firing properly.
Having a page with <body onpagehide="alert('test');"></body> and the alert is still called when closing the tab.
I think the pagehide is always fired also when unload is fired.
Attached patch A bit of debugging (obsolete) — — Splinter Review 
I added some NS_WARNINGs to the current FormFillController.cpp code to see when events are getting triggered.

If I load a test page and then close the tab, only BeforeUnload is called. [and *not* pagehide]
If I load a test page and then go to google.com, BeforeUnload and pagehide are fired.
If I load google.com and then a test page, going back to google.com fires BeforeUnload and pagehide.

I remember testing this before, and found that JS code was getting the right events but this C++ code in satchel wasn't. Something buggy with chromeEventHandler as an event target, maybe? [See ::AddWindowListeners()]
Comment on attachment 282540 [details] [diff] [review]
patch

Thanks for digging this out, Justin.
Indeed, I only tested in js code.
Attachment #282540 - Flags: review?(dolske)
Depends on: 398502
I suppose this bug and bug 384230 are different facets of the same problem, and although not technically duplicates can get fixed at the same time.

I filed 397753 on the pagehide event being inconstantly fired, as you noted in comment #2.
Blocks: 384230
So, bug 376563 has 'solved' the issue of ::Unload not being called. This is basically an updated version of Martijn's patch, with the ::RemoveWindowListeners handing what ::Unload was formally expected to. [It actually already calls StopControllingInput(), so the only change is the addition of the pwmgr code that bug 384230 sought to add.]
Attachment #282540 - Attachment is obsolete: true
Attachment #282969 - Attachment is obsolete: true
Attachment #286039 - Flags: review?(martijn.martijn)
Comment on attachment 286039 [details] [diff] [review]
Patch for review, v.2

The patch seems to work fine.
I haven't checked if it fixes bug 384230, though. But according to bug 376563, comment 8, it should, right?

Looking at the code, it seems to me that the click addEventListener can be remove too, since it doesn't seem to do anything useful. But that's not necessarily a concern for this patch.
Attachment #286039 - Flags: review?(martijn.martijn) → review+
Attachment #286039 - Flags: review?(gavin.sharp)
Gavin, can you review the patch?
Assignee: martijn.martijn → dolske
Attachment #286039 - Flags: review?(gavin.sharp) → review+
Attachment #286039 - Flags: approval1.9?
Dolske: you asked for approval, can I get a quick risk/reward summary? Moving handlers like this triggers my "hmmm" senses.
The primary reward is that it ends up fixing bug 384230, which is a memory leak.

I think it's a fairly low risk, there was some confusion early on because the existing code was broken in a misleading way, but once that was figured out the fix was fairly obvious. There are a limited number of ways/states to exercise these changes, and I checked them when writing it.
Comment on attachment 286039 [details] [diff] [review]
Patch for review, v.2

a=beltzner for 1.9
Attachment #286039 - Flags: approval1.9? → approval1.9+
w00t!

Checking in toolkit/components/satchel/src/nsFormFillController.cpp;
  new revision: 1.99; previous revision: 1.98
Checking in toolkit/components/satchel/src/nsFormFillController.h;
  new revision: 1.18; previous revision: 1.17

I also tested this again to make sure the various ways of trigging a page close/hide were handled properly, and that all the code paths here were executed.

[Note for future reference: The |if (mFocusedInput)| case was tricky to trigger: I got it by having the Google search input field focused, and then selecting a bookmark from the Bookmarks menu. Seems like other was of navigating away from the page defocused the input first, and so this code wasn't triggered.]
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite-
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
Component: Satchel → Form Manager
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: