Closed Bug 826115 Opened 7 years ago Closed 7 years ago

warnings about rollups too noisy

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox19 + fixed

People

(Reporter: heycam, Assigned: mak)

References

Details

(Keywords: regression, Whiteboard: [qa?])

Attachments

(1 file, 1 obsolete file)

There are some noisy warnings emanating from the

  NS_ENSURE_TRUE(rollupWidget, false);

line in nsWindow.cpp and in corresponding files for other platforms.  If that's not an exceptional condition, we should just be doing `if (!rollupWidget) return false;`.
This appears to have been introduced as part of Bug 819888.
Remove the warning, the previous code was just null checking this so looks like it's not needed.
Attachment #697931 - Flags: review?(jmathies)
Attachment #697931 - Flags: review?(jmathies) → review+
There is an equivalent problem under OS X. Don't land the patch yet, I'll find it for you (give me a minute or two)...
https://hg.mozilla.org/integration/mozilla-inbound/rev/324f9002ab8a

Sure, we can take separate patches per platform.
Assignee: nobody → mak77
Whiteboard: [leave open]
Attachment #697931 - Attachment description: patch v1.0 → Windows patch v1.0
Attached patch OS X Patch 1.0 (obsolete) — Splinter Review
This isn't ready to land. Although the code should be equivalent, applying this patch causes a segfault down inside mozStorageService.cpp when the browser terminates. I suspect the segfault is an unrelated, pre-existing defect that this change somehow exposes, but I don't have time to chase that down right now.
(In reply to Adam Roach [:abr] from comment #5)
> This isn't ready to land. Although the code should be equivalent, applying
> this patch causes a segfault down inside mozStorageService.cpp when the
> browser terminates. I suspect the segfault is an unrelated, pre-existing

That shouldn't be related to your patch. I have seen dozen of assertions and crashes for that with the latest debug tinderbox build on 10.7 earlier today.
(In reply to Henrik Skupin (:whimboo) from comment #6)
> (In reply to Adam Roach [:abr] from comment #5)
> > This isn't ready to land. Although the code should be equivalent, applying
> > this patch causes a segfault down inside mozStorageService.cpp when the
> > browser terminates. I suspect the segfault is an unrelated, pre-existing
> 
> That shouldn't be related to your patch. I have seen dozen of assertions and
> crashes for that with the latest debug tinderbox build on 10.7 earlier today.

Yeah, I agree. But right now, that crash will erroneously bisect to this patch. I want to wait for the builds to be more stable before we try landing this. In any case, I've got some higher priority bugs to chase right now, and this is just an annoyance. I'll circle back around to it early next week.
Comment on attachment 697949 [details] [diff] [review]
OS X Patch 1.0

Review of attachment 697949 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsToolkit.mm
@@ +167,5 @@
>    nsIRollupListener* rollupListener = nsBaseWidget::GetActiveRollupListener();
>    NS_ENSURE_TRUE(rollupListener, event);
>    nsCOMPtr<nsIWidget> rollupWidget = rollupListener->GetRollupWidget();
> +  if (!rollupWidget)
> +    return event;

Looks like this was already a simple check and was explicitly converted to a warning NS_ENSURE. I wonder if that was with a reason.
(In reply to Marco Bonardo [:mak] from comment #8)
> Comment on attachment 697949 [details] [diff] [review]
> OS X Patch 1.0
> 
> Review of attachment 697949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/cocoa/nsToolkit.mm
> @@ +167,5 @@
> >    nsIRollupListener* rollupListener = nsBaseWidget::GetActiveRollupListener();
> >    NS_ENSURE_TRUE(rollupListener, event);
> >    nsCOMPtr<nsIWidget> rollupWidget = rollupListener->GetRollupWidget();
> > +  if (!rollupWidget)
> > +    return event;
> 
> Looks like this was already a simple check and was explicitly converted to a
> warning NS_ENSURE. I wonder if that was with a reason.

This was the result of a simple copy paste error from the check above. This form of check is fine.
(In reply to Adam Roach [:abr] from comment #5)
> Created attachment 697949 [details] [diff] [review]
> OS X Patch 1.0
> 
> This isn't ready to land. Although the code should be equivalent, applying
> this patch causes a segfault down inside mozStorageService.cpp when the
> browser terminates.

You may do a try landing, but I don't see anything dangerous in your patch honestly and I don't see how it could change the frequency of a segfault.
re-assigning to :abr to handle the OSX part. I think if we get a green try run out of that we can just proceed.
Assignee: mak77 → adam
We should probably close this out and clone this over for osx work now that the windows fix is tracked for beta.
(In reply to Jim Mathies [:jimm] from comment #14)
> We should probably close this out and clone this over for osx work now that
> the windows fix is tracked for beta.

Sure, sounds good to me. I'd hoped to finish figuring out what's going on with the crash and land this, but I've been tied up on other things recently.
Blocks: 831342
cloned to Bug 831342
Assignee: adam → mak77
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Attachment #697949 - Attachment is obsolete: true
Marking this [qa?]. Is there anything QA can help with?
Whiteboard: [qa?]
You need to log in before you can comment on or make changes to this bug.