826115 - warnings about rollups too noisy

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Cameron McCormack (:heycam)]

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;`.

Comment 1

[Adam Roach [:abr]]

This appears to have been introduced as part of Bug 819888.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Windows patch v1.0

Remove the warning, the previous code was just null checking this so looks like it's not needed.

Comment 3

[Adam Roach [:abr]]

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)...

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/324f9002ab8a

Sure, we can take separate patches per platform.

Comment 5

[Adam Roach [:abr]]

Attachment: 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. 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.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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.

Comment 7

[Adam Roach [:abr]]

(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 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 9

[Jim Mathies [:jimm]]

(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.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(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.

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/324f9002ab8a

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 13

[Jim Mathies [:jimm]]

windows fix:
https://hg.mozilla.org/releases/mozilla-beta/rev/aad6dcd323c3

Comment 14

[Jim Mathies [:jimm]]

We should probably close this out and clone this over for osx work now that the windows fix is tracked for beta.

Comment 15

[Adam Roach [:abr]]

(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.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

cloned to Bug 831342

Comment 17

[Manuela Muntean [Away]]

Marking this [qa?]. Is there anything QA can help with?