warnings about rollups too noisy

RESOLVED FIXED

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: heycam, Assigned: mak)

Tracking

({regression})

Trunk
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox19+ fixed)

Details

(Whiteboard: [qa?])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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

6 years ago
This appears to have been introduced as part of Bug 819888.
Keywords: regression
Created attachment 697931 [details] [diff] [review]
Windows patch v1.0

Remove the warning, the previous code was just null checking this so looks like it's not needed.
Attachment #697931 - Flags: review?(jmathies)

Updated

6 years ago
Attachment #697931 - Flags: review?(jmathies) → review+

Comment 3

6 years ago
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]
(Assignee)

Updated

6 years ago
Attachment #697931 - Attachment description: patch v1.0 → Windows patch v1.0

Comment 5

6 years ago
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. 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.

Comment 7

6 years ago
(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.

Comment 9

6 years ago
(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

Comment 13

6 years ago
windows fix:
https://hg.mozilla.org/releases/mozilla-beta/rev/aad6dcd323c3
status-firefox19: --- → fixed
tracking-firefox19: --- → ?

Updated

6 years ago
tracking-firefox19: ? → +

Comment 14

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

Comment 15

6 years ago
(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.
(Assignee)

Updated

6 years ago
Blocks: 831342
cloned to Bug 831342
Assignee: adam → mak77
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Whiteboard: [leave open]
(Assignee)

Updated

6 years ago
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.