Closed Bug 340882 Opened 14 years ago Closed 14 years ago

Invalid pointer in RemoveWindowListeners

Categories

(Toolkit :: Form Manager, defect, P1, critical)

1.8 Branch
x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta1

People

(Reporter: brettw, Assigned: brettw)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [need testcase])

Attachments

(3 files)

932 nsFormFillController::RemoveWindowListeners(nsIDOMWindow *aWindow)
933 {
934   if (!aWindow)
935     return;
936 
937   StopControllingInput();
938   
939   nsCOMPtr<nsPIDOMWindow> privateDOMWindow(do_QueryInterface(aWindow));
940   nsIChromeEventHandler* chromeEventHandler;
941   if (privateDOMWindow)
942     chromeEventHandler = privateDOMWindow->GetChromeEventHandler();
943   
944   nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(chromeEventHandler));

The pointer declared in line 940 might be anything, and it goes into do_QueryInterface.

This code was originally written by bryner with a "= null" in the variable definition. It looks like ben then merged the aviary branch incorrectly which didn't have this change, and the NULL initialization was lost.

This might be the cause of bug 339861. Perhaps we've always happened to have a NULL at this stack location before, and changing back to Mork made it be something else? It would not, however, explain the runtime crashes that people were reporting with the bug.
Severity: normal → critical
Priority: -- → P1
Summary: Possible invalid pointer in RemoveWindowListeners → Invalid pointer in RemoveWindowListeners
Target Milestone: --- → mozilla1.8.1beta1
This patch also fixes the rest of the warnings in the satchel component. I found this bug because of a warning. Perhaps this wasn't found earlier because there are several others.
Attachment #224922 - Flags: first-review?(bryner)
Attachment #224922 - Flags: approval-branch-1.8.1?
Attachment #224922 - Flags: approval-branch-1.8.1? → approval-branch-1.8.1?(benjamin)
Why are you using javascripts? Have you download and installed it sperate from firefox?
Attachment #224922 - Flags: first-review?(bryner) → first-review+
*** Bug 338361 has been marked as a duplicate of this bug. ***
Attachment #224922 - Flags: approval-branch-1.8.1?(benjamin) → approval-branch-1.8.1+
closed lol
Fixed on 1.8 branch and trunk.
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
It would be good if we could get the NULL initialization for 1.5 (the warning fixes don't matter).
Flags: blocking1.8.0.5?
Flags: blocking1.8.0.5? → blocking1.8.0.5+
For 1.8.0.5 we'd just like the initialization fix.
Comment on attachment 224922 [details] [diff] [review]
Patch for this bug & all warnings

>         nsIFrame* frame;
>-        nsresult rv = presShell->GetPrimaryFrameFor(content, &frame);
>+        presShell->GetPrimaryFrameFor(content, &frame);

You should initialize frame to null, too, especially since you're
getting rid of rv rather than adding a check for whether 
GetPrimaryFrameFor failed or not.
You're right, I should have checked the return value instead of removing the return variable. It seems this is already fixed on trunk. Perhaps we want this change for 1.5.0 as well, since GetPrimaryFrameFor might fail and we would crash.
Attachment #225074 - Flags: first-review?(bryner)
Attachment #225074 - Flags: approval-branch-1.8.1?(bryner)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #225074 - Flags: first-review?(bryner)
Attachment #225074 - Flags: first-review+
Attachment #225074 - Flags: approval-branch-1.8.1?(bryner)
Attachment #225074 - Flags: approval-branch-1.8.1+
The NULL-check patch is on 1.8 branch. It is not necessary on trunk.
Status: REOPENED → ASSIGNED
FIXED per comment 5 and comment 10
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Comment on attachment 225074 [details] [diff] [review]
Another patch to fix NULL checking

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #225074 - Flags: approval1.8.0.5+
Comment on attachment 224922 [details] [diff] [review]
Patch for this bug & all warnings

a=sicking for drivers for 1.8.0 branch of JUST the second chunk of the nsFormFillController.cpp (i.e. just the null-initializer)
Comment on attachment 224922 [details] [diff] [review]
Patch for this bug & all warnings

>Index: nsFormFillController.cpp
>===================================================================
>@@ -962,17 +962,17 @@ void
> nsFormFillController::RemoveWindowListeners(nsIDOMWindow *aWindow)
> {
>   if (!aWindow)
>     return;
> 
>   StopControllingInput();
>   
>   nsCOMPtr<nsPIDOMWindow> privateDOMWindow(do_QueryInterface(aWindow));
>-  nsIChromeEventHandler* chromeEventHandler;
>+  nsIChromeEventHandler* chromeEventHandler = nsnull;
>   if (privateDOMWindow)
>     chromeEventHandler = privateDOMWindow->GetChromeEventHandler();
>   
>   nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(chromeEventHandler));
> 
>   if (!target)
>     return;
> 

We want just this bit of the patch on 1.8.0 branch
approved for 1.8.0 branch, a=dveditz for drivers
Attachment #224922 - Flags: approval1.8.0.5+
Checked in on 1.8.0 branch
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.5
I didn't mean to verify it...
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
v.fixed on 1.8.0 branch by code inspection  If there is any way to verify this fix with a testcase, please let us know.
Whiteboard: [need testcase]
Component: Satchel → Form Manager
You need to log in before you can comment on or make changes to this bug.