Closed Bug 893973 Opened 11 years ago Closed 9 years ago

Non e10s crash in -[ChildView keyDown:] [Mac]

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Version:
25 Branch
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla25
Tracking Flags:
Tracking Status
e10s - ---
firefox24 --- unaffected
firefox25 --- affected

People

(Reporter: scoobidiver, Assigned: masayuki)

References

Details

(Keywords: crash, regression, Whiteboard: [tbird crash])

Crash Data

Attachments

(4 files, 3 obsolete files)

Patch
2.72 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
nsIWidget::SetInputContext() should check it has focus before calling secure event input API
3.57 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
Additional debugging patch rev2 (what I relanded)
3.44 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
Add more debug info, rev1
1.47 KB, patch
masayuki
: review+
Details | Diff | Splinter Review 
If first showed up in 25.0a1/20130701 but is discontinuous across builds.

Signature 	-[ChildView keyDown:] More Reports Search
UUID 	086b709e-00e9-48a9-935d-b70692130715
Date Processed	2013-07-15 18:52:23.409452
Uptime	6120
Last Crash	6270 seconds before submission
Install Age 	6120 since version was first installed.
Install Time 	2013-07-15 17:10:19
Product 	Firefox
Version 	25.0a1
Build ID 	20130715030202
Release Channel 	nightly
OS 	Mac OS X
OS Version 	10.8.4 12E55
Build Architecture 	amd64
Build Architecture Info 	family 6 model 58 stepping 9 | 4
Crash Reason 	EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash Address 	0x0
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x 166GL Layers! GL Context? GL Context+ GL Layers+ 

Frame 	Module 	Signature 	Source
0 	XUL 	-[ChildView keyDown:] 	widget/cocoa/nsChildView.mm
1 	AppKit 	AppKit@0x23b020 	
2 	libsystem_c.dylib 	libsystem_c.dylib@0x1a195 	
3 	libsystem_c.dylib 	libsystem_c.dylib@0x1a195 	
4 	HIToolbox 	HIToolbox@0x7b5cf 	
5 	HIToolbox 	HIToolbox@0x7f7f7 	
6 	HIToolbox 	HIToolbox@0x7f6f8 	
7 	HIToolbox 	HIToolbox@0x8c4d1 	
8 	XUL 	-[BaseWindow sendEvent:] 	widget/cocoa/nsCocoaWindow.mm
9 	XUL 	-[ToolbarWindow sendEvent:] 	widget/cocoa/nsCocoaWindow.mm
10 	AppKit 	AppKit@0x236644 	

More reports at:
https://crash-stats.mozilla.com/report/list?product=Firefox&signature=-[ChildView+keyDown%3A]
These are "caused" by the patch for bug 802686, which changed an assertion into a deliberate crash:

http://hg.mozilla.org/mozilla-central/annotate/ff0a372e3170/widget/cocoa/nsChildView.mm#l5201
http://hg.mozilla.org/mozilla-central/rev/55c1f447549d

I'm denied access to that bug, even though I think I belong to all the relevant security groups.  I'll try to get access and find out what's going on.
Justin, you have hg blame for the change that triggers these crashes.  And you appear to be the author of at least part of the patch for bug 802686.

This bug's crashes are currently the #7 topcrasher on the 25 branch.

Did you expect to see them in such large numbers?

Is the underlying security bug drop-dead important, or is it more important to reverse your patch (at least on OS X) and stop these crashes.

I realize I'm asking about a security bug "in the open" (since I don't have access to bug 802686), and you'll need to be careful what you say in your reply.

If you can, please give me access to bug 802686 :-)
Flags: needinfo?(justin.lebar+bug)
> These are "caused" by the patch for bug 802686, which changed an assertion into a 
> deliberate crash:

Sorry, my commit has the wrong bug number.  I have no idea what that bug is; I asked someone on sg and they also couldn't see it.

The relevant bug is 820686.  You'll see there that we changed what was /undefined behavior/ into a crash.  Congratulations: This is the first crash we found.

I have no idea what the right thing is to do to fix this bug, but it's definitely not to put MOZ_ASSUME_NOT_REACHED back in there.
Flags: needinfo?(justin.lebar+bug)
> I have no idea what the right thing is to do to fix this bug, but
> it's definitely not to put MOZ_ASSUME_NOT_REACHED back in there.

I agree.

But can't we just use an assertion (a real one) instead of crashing?
That is, unless the security implications of not crashing are too
severe.

(Until you put me straight, that's what I assumed
MOZ_ASSUME_NOT_REACHED was.)
> This bug's crashes are currently the #7 topcrasher on the 25 branch.

I should have clarified that this among Mac crashes.
> But can't we just use an assertion (a real one) instead of crashing?

I suppose you could, although do you really want to add an assertion that you know is sometimes false?

Anyway that's not a question for me to answer; I made the moz_not_reached --> moz_crash change without consideration for the security implications of each callsite on its own.
> do you really want to add an assertion that you know is sometimes false?

I'd prefer it to crashing, if the security implications aren't too severe.

Masayuki, I'd guess that the security problem we're dealing with isn't too severe.  What do you think?
> I'd prefer it to crashing, if the security implications aren't too severe.

Me too, but why isn't handling the heretofore unexpected situation an option?  Do we even understand what this assertion is, and why it's tripping?  If this were a module I controlled, I'd certainly expect an attempt at that before we changed any code.

Also, this code seems to be assuming that RELEASE_BUILD is defined when we're in a release build, but that doesn't seem to be the case.  I wonder if there's other code around here that is ifdef'ed !defined(RELEASE_BUILD) but that's actually running in release builds.
> why isn't handling the heretofore unexpected situation an option?

Off the top of my head, I don't know whether it is or not.  But I seem to remember lots of OS-level flakiness on this issue.  I hope Masayuki knows more about it than I do.
The crash means:

1. The input events can be listen by other processes even when our password field has focus
2. The input events can not be listen by other process even when our password field does not have focus.

If the crash reason is #1, the crash can protect the user from key logger. On the other hand, if the reason is #2, it might not be worthwhile to crash.

I wrote the check for prompting the testers to report the bug...
(In reply to Justin Lebar [:jlebar] from comment #8)
> Also, this code seems to be assuming that RELEASE_BUILD is defined when
> we're in a release build, but that doesn't seem to be the case.  I wonder if
> there's other code around here that is ifdef'ed !defined(RELEASE_BUILD) but
> that's actually running in release builds.

Yes, the crash only happens on the builds for our testers.

If release build fails to manage the secure input mode, it may allow key loggers to steal the password when the user types it. However, such bug was there before bug 807893. Therefore, I think that release build don't need to crash. However, for getting bug reports, I'd prefer the builds for testers to crash when they meet the bug.
Attached patch Patch — — Splinter Review 
I found a case that the crash occurs unexpectedly.


1. Firefox is deactivated and the last focused element is password field.
2. IME or a11y tool steals focus from Firefox for text input UI which generates key events on the latest active window.

If both conditions occur at same time, the keydown event which is generated by the other window causes crash. Since the input context indicates password editor but secure event input mode was disabled when the window is deactivated.
# Although, I don't know actual tool for such case.

This patch checks the focus status. And this patch separates MOZ_CRASH() for each case for distinguishing what causes the crash.

This may not be fix this bug completely. However, this patch will give new hint for us.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #776394 - Flags: review?(smichaud)
> I found a case that the crash occurs unexpectedly.

Could you give more detail?  For example a specific password field, and specific steps to take using IME.

I was unable to reproduce using your existing STR from comment #12.
Comment on attachment 776394 [details] [diff] [review]
Patch

Looks good to me.  Hopefully this will cut down on the number of crashes.

But ultimately I'd like to understand why we're having these problems with "secure input", and what we can do about them.

If we have bugs that are contributing to the problem, we should fix them.  If the OS's implementation has bugs, we should try to work around them.

But if the OS's implementation is too flaky or badly designed, we may need to stop using it.

If I remember right, an earlier version of Apple's "secure input" was crippled by turning on "Enable access for assistive devices" in the Accessibility pref panel.  Is that still true?
Attachment #776394 - Flags: review?(smichaud) → review+
(In reply to Steven Michaud from comment #13)
> > I found a case that the crash occurs unexpectedly.
> 
> Could you give more detail?  For example a specific password field, and
> specific steps to take using IME.
> 
> I was unable to reproduce using your existing STR from comment #12.

As I mentioned comment 12, I don't find any actual example of such tool. But I have no idea for reproducing this bug except the situation (I assume that nsIWidget::NotifyIME() is called as expected).
(In reply to Steven Michaud from comment #14)
> But ultimately I'd like to understand why we're having these problems with
> "secure input", and what we can do about them.
> 
> If we have bugs that are contributing to the problem, we should fix them. 

Yes, we should do it. Therefore, I separate MOZ_CRASH() for each case.

> If the OS's implementation has bugs, we should try to work around them.

I don't think that this is caused by OS's bug since TextInputHandler just manages the count of calls of the APIs. So, TextInputHandler::IsSecureEventInputEnabled() is NOT a wrapper of API. It just checks if the API call count is not 0.

> But if the OS's implementation is too flaky or badly designed, we may need
> to stop using it.
> 
> If I remember right, an earlier version of Apple's "secure input" was
> crippled by turning on "Enable access for assistive devices" in the
> Accessibility pref panel.  Is that still true?

I don't know the "earlier version"... But in my scenario, I feel current API design is strange. Current API enables/disables secure event input mode all over the system. Therefore, every application needs to disable the secure event input mode at deactivated. I.e., sent key events are never protected after deactivated.
https://hg.mozilla.org/mozilla-central/rev/fd656ca11e99
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment on attachment 776394 [details] [diff] [review]
Patch

I just realized this patch has a problem (like the code it replaced) -- the output of MOZ_CRASH doesn't get into Breakpad/Socorro crash reports.  Which means that deliberately crashing on these error conditions doesn't do us any good, especially if we don't have any way to reproduce these crashes.

xpcom_runtime_abort output does get into the App Notes field of Socorro crash reports.  See for example bp-fc027be0-770b-474d-93ce-1876b2130717.  I'm going to try to find a way to make MOZ_CRASH behave the same way.
>  Which means that deliberately crashing on these error conditions doesn't do us any good,

Is it not sufficient to get a line number out of the crash report?

> I'm going to try to find a way to make MOZ_CRASH behave the same way.

I don't have strong feelings about this, but I think that waldo wanted to be sure that MOZ_CRASH always crashed safely, even in the face of arbitrarily-corrupted memory.  It might be difficult to accomplish this while also calling into breakpad.
I echo the line-number point.

I assume the breakpad thing is just copying the provided chars into a static array in app memory, that's been registered with breakpad.  That should be doable enough, with safety, I think, unless I'm missing something.
> I assume the breakpad thing is just copying the provided chars into a static array in app memory, 
> that's been registered with breakpad.

Assuming that the access to this array doesn't go through the GOT.  Which I'd guess it probably does, due to PIC, although I'm not very familiar with this stuff.
> Is it not sufficient to get a line number out of the crash report?

I hadn't thought of that.  I suppose it is.

I'd already changed my mind about using MOZ_CRASH, after noticing that its docs say it needs to crash safely.  (Instead I was intending to call CrashReporter::AppendAppNotesToCrashReport() directly, then call an unaltered MOZ_CRASH("...").)

But you're right, probably nothing more needs to be done here.

However, I'll revisit this if I find some -[ChildView keyDown:] crash reports without line numbers.
> I'd already changed my mind about using MOZ_CRASH ...

I'd already changed my mind about *changing* MOZ_CRASH (for all consumers).

In this bug's case, I doubt very much we have to worry about crash safety.  But I think it's wise to be worried about it in the general case (for any consumer of MOZ_CRASH).
It's not fixed. See bp-eee2bd70-d9b1-4e15-aac9-78dd82130718 with a slightly different stack trace.
Status: RESOLVED → REOPENED
status-firefox25: fixed → affected
Resolution: FIXED → ---
> bp-eee2bd70-d9b1-4e15-aac9-78dd82130718

And I see this report has no line number -- possibly because it comes from one of our test machines.

I'll write a patch that uses CrashReporter::AppendAppNotesToCrashReport() to add information about these crashes to Breakpad crash reports.
>> bp-eee2bd70-d9b1-4e15-aac9-78dd82130718
>
> And I see this report has no line number -- possibly because it
> comes from one of our test machines.

Actually it *does* have a line number (5217), though that appears only
in the raw dump.

https://hg.mozilla.org/mozilla-central/annotate/16375716cc69/widget/cocoa/nsChildView.mm#l5217

But I'll still write my patch, and make it log additional, potentially
useful information to the Breakpad crash reports' app notes
(information that can't be deduced from the line number).

The bottom line here is that I don't think we can afford to crash on
these errors, even in mozilla-central nightlies, if 1) there are too
many of them and 2) there's no prospect of our fixing (or working
around) the underlying bugs quickly.

Bugs that we can't reproduce we won't be able to fix quickly (or
well).  But hopefully the additional information that I'll add to the
crash reports' app notes will allow us to figure out how to reproduce
at least some of these bugs.
Hmm, but the crash indicates security bug since password file has focus without secure event input mode. So, I really want the STR...
Attached patch Additional debugging patch (obsolete) — — Splinter Review 
Here's my patch.  It adds information to the Breakpad crash reports' app notes field for crashes that are triggered by the original patch's calls to MOZ_CRASH.

It's conceivable that the information from TextInputHandlerBase::IsSecureEventInputEnabled()'s call to CrashReporter::AppendAppNotesToCrashReport() might also add information to other kinds of crash reports.  But I don't think that's a bad thing.

Masayuki, can you think of any other information that'd be useful for us to have in the app notes field of this bug's crash reports?
Attachment #778014 - Flags: review?(ted)
Attachment #778014 - Flags: review?(masayuki)
Attachment #778014 - Attachment is patch: true
By the way, here's another post-patch crash at a different address:
bp-f949ed04-c271-42e8-8a6c-e45ae2130718

So we (apparently) are dealing with (at least) two different problems.
(Following up #30)

Or bp-eee2bd70-d9b1-4e15-aac9-78dd82130718's line number (5217) is for a different revision of nsChildView.mm

All the more reason to land my app notes patch (or some version of it).
Whiteboard: [leave open]
(In reply to Steven Michaud from comment #30)
> By the way, here's another post-patch crash at a different address:
> bp-f949ed04-c271-42e8-8a6c-e45ae2130718
I doubt Nightly-UX already contains the patch.
When you follow bp-f949ed04-c271-42e8-8a6c-e45ae2130718's link to the line in nsChildView.mm where the crash happened, you see Masayuki's latest patch.
And yes, this bug's patch did land on the UX branch in time to get into today's ux branch nightly:
https://hg.mozilla.org/projects/ux/rev/6fdc29dabfb3
Comment on attachment 778014 [details] [diff] [review]
Additional debugging patch

+ ... (NS_LITERAL_CSTRING("\nBug 893373: ") +

Just noticed that I got the bug number wrong here.  It should be 893973.

I'll fix this when I land the patch or in my next revision.
Attachment #778014 - Flags: review?(ted) → review+
Comment on attachment 778014 [details] [diff] [review]
Additional debugging patch

Although, you might misunderstand the TextInputHandlerBase::IsSecureEventInputEnabled(). Even if the secure event input mode is wrong for us actually, it's not problem. The method just return whether our responsive calls are expected or not.

If you think you don't need the change of TextInputHandlerBase::IsSecureEventInputEnabled() is not necessary, please remove it. But it's not matter.

r=me.
Attachment #778014 - Flags: review?(masayuki) → review+
As far as I understand, nsIWidget::SetInputContext() is called only when the widget has focus in OS level. However, I may misunderstand. This patch protects secure event input API calls from unfocused widget.

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=a82b4cd14c5e
Here's what I actually landed.

Masayuki, I took your advice and dropped my changes to TextInputHandlerBase::IsSecureEventInputEnabled().  I also add more information to Breakpad's app notes, hoping that it will be useful.

Carrying forward r+ from ted and masayuki.
Attachment #778014 - Attachment is obsolete: true
Attachment #778543 - Flags: review+
Weird.  It built fine on my system.

I do a tryserver build before submitting it again.
Relanded with minor changes on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05fd9daf1492

These changes worked find on tryserver.
Carrying over Ted's and Masayuki's r+s.
Attachment #778543 - Attachment is obsolete: true
Attachment #778672 - Flags: review+
Comment on attachment 778350 [details] [diff] [review]
nsIWidget::SetInputContext() should check it has focus before calling secure event input API

Let's check at calling secure event input API from SetInputContext(). I assume that SetInputContext() is called only when the widget has focus. However, if this is wrong, non-focused widget may break the secure event input state of focused widget.
Attachment #778350 - Flags: review?(smichaud)
Here is an error message:
"Bug 893973: A password editor has focus, but not in secure input modeview [ChildView 0x10ea0b6e0, gecko child 0x10cdc5800, frame {{0, 0}, {1440, 852}}], window [<ToolbarWindow: 0x10d6b6c50>], key event [NSEvent: type=KeyDown loc=(0,874) time=77335.0 flags=0x100108 win=0x10d6b6c50 winNum=5653 ctxt=0x0 chars="w" unmodchars="w" repeat=0 keyCode=13], window is key 1, app is active 1"
Comment on attachment 778350 [details] [diff] [review]
nsIWidget::SetInputContext() should check it has focus before calling secure event input API

Looks fine to me, at least as an experiment.  We should keep this bug open after it lands, though.

Sorry for the delay, but I was on vacation last week.
Attachment #778350 - Flags: review?(smichaud) → review+
(In reply to comment #46)

I've looked at several of these error messages, and in each of them the ChildView object (and its NSWindow) is quite large.  So we might be in fullscreen mode when these crashes happen.

In a bit I'll post a patch that logs more debugging info.
Attached patch Add more debug info (obsolete) — — Splinter Review 
Masayuki, let me know if you can think of any other information we could log that might be useful.
Attachment #782633 - Flags: review?(masayuki)
Comment on attachment 782633 [details] [diff] [review]
Add more debug info

Perhaps, this patch should land first before my experimental patch.
Attachment #782633 - Flags: review?(masayuki) → review+
Depends on: 900007
Comment on attachment 782633 [details] [diff] [review]
Add more debug info

This is going to get backed out to fix bug 900007.

I'll figure out later what's going on, and repost a fixed patch.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #52)
> https://hg.mozilla.org/mozilla-central/rev/197b12188f3a

I backed out this patch for causing bug 900007:

https://hg.mozilla.org/mozilla-central/rev/fd03bb4d1e48
Hmm, it's very strange. I don't have any trouble on 10.7. Is it a bug of 10.8?
I installed 10.8, but I cannot reproduce the bug on my environment, why???
Hi,

I began seeing Bug 900007 in MacOS 10.6.8+ as well, after the tinderbox build with it came out.  In fact I was going to try reporting the same problem.

The backout seems to have made the arrow keys useful again now.  ;)

FWIW, I've been following/using the m-c tinderbox builds (mainly so to see if the Nightly builds have already been 'fixed' <g>), and I suddenly experience the bug starting late Tuesday July 30 after a m-c build came out.  I noticed this due to c-c (Thunderbird tinderbox) having a build that just happened to 'land' right-smak-dab on 900007 earlier overnight.

(BTW, I am one of those where 10.6 is EOL for me, and I cannot afford a newer machine to let me continue [blame Apple for not giving us 64-bit EFI/BIOS on these models, and I won't mess with e.g. Chameleon to provide a workaround].  Me being on disability and retired [with nearly four-decades work on record].  To boot, I already have plans to jump ship, wanting to do full F/OSS, which Apple has sincerely forced me to do.  Sorry for the banter, if only to 'prove' I am very sure of my reckonings here.)
I'm on 10.8.4, FWIW.
I can reproduce bug 900007 on OS X 10.8.4.  I haven't yet tried on any other version of the OS.

Since I can repro on at least one of my machines, I should be able to figure out what triggered the bug.  I'll do that later today.
What caused bug 900007 seems to have been the call to [window isZoomed].  At least getting rid of that call makes the problem go away.  That's what this patch does.

It's hard to say why this makes a difference.  But the OS does call various methods of NSWindow and NSWindowDelegate during the call to isZoomed -- notably windowShouldZoom:toFrame:, which we implement in WindowDelegate.

If the call to [NSWindow isZoomed] were more important, I'd spend more time figuring out why it blows up in our face.  As best I can tell it isn't important, so I'll just drop the call.
Attachment #782633 - Attachment is obsolete: true
Attachment #784632 - Flags: review?(masayuki)
Comment on attachment 784632 [details] [diff] [review]
Add more debug info, rev1

Unfortunately, nobody reproduced this bug with the previous patch :-(
Attachment #784632 - Flags: review?(masayuki) → review+
(In reply to comment #61)
> Created attachment 784632 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=784632&action=edit
> Add more debug info, rev1
> 
> What caused bug 900007 seems to have been the call to [window isZoomed].  At
> least getting rid of that call makes the problem go away.  That's what this
> patch does.
> 
> It's hard to say why this makes a difference.  But the OS does call various
> methods of NSWindow and NSWindowDelegate during the call to isZoomed -- notably
> windowShouldZoom:toFrame:, which we implement in WindowDelegate.
> 
> If the call to [NSWindow isZoomed] were more important, I'd spend more time
> figuring out why it blows up in our face.  As best I can tell it isn't
> important, so I'll just drop the call.

Can you please tell me more about the mechanics of this failure?  Was this an exception?  I tried to figure out how I can debug this before I backed out the patch but I did not really know where to start (since I'm not familiar with Objective C).

Thanks!
> Was this an exception?

No, it wasn't.  If it had been you'd have seen an error in the system console, and I didn't.
If you really want to figure out what was going on, probably the best place to start is to look at -[NSWindow isZoomed] in a disassembler :-)

I really like the Hopper Disassembler (http://www.hopperapp.com/), which came out fairly recently.
(In reply to comment #67)
> If you really want to figure out what was going on, probably the best place to
> start is to look at -[NSWindow isZoomed] in a disassembler :-)

Yeah, that's what I figured.  But my problems are much deeper than that; I couldn't figure out how to specify the function name in gdb to break on it!  :-)  I guess I'll stick to C++ for now!
So far three reports have shown up made with my latest debug logging in them:

bp-f515c020-845d-4aff-adc7-77aec2130805
bp-985c0ba2-9bd2-4988-957f-5c6b92130805
bp-f88d132a-0027-4995-b3d4-1eaf22130805

None of them reports fullscreen mode on.

I'll keep thinking about this.  But at least for the time being I'm out of ideas.

Masayuki, please to think of other debug information it might be useful to collect.
"please to think" -> "please try to think" :-)
I still have no idea. However, I'm guessing that attachment 778350 [details] [diff] [review] will _prevent_ this crash. If so, there might be a bug on XP level (editor, focus manager or IME state manager).
I feel that the number of crash users is only a few users according to the installed add-on list. So, this crash might depend on specific web site or add-on.
> I'm guessing that attachment 778350 [details] [diff] [review] will _prevent_ this crash.

Go ahead and land it, and give it a try.
Yes.  Every Socorro crash log but one over the last two weeks (bp-c2325e44-7e6a-4a3b-b690-3dd6b2130730) lists onepassword@agilebits.com as one of the installed extensions.
I installed the 1Password Firefox browser extension (https://agilebits.com/extensions/mac/index.html, which is unusable on its own) and tried to reproduce this bug's crashes by pressing Cmd-\ (which is the browser extension's default "hotkey").

I saw three crashes, one of which (interestingly) was on Cmd-Shift-\, and another of which (very interestingly) was an EMPTY crash.

bp-b6d0e3e8-0750-4f6b-b5b2-2dbc02130808
bp-903e6177-2684-4bbf-9b50-606f72130808
bp-0161ad5a-0bac-42dd-8bb0-e757b2130808

But I haven't (yet) been able to figure out how to reproduce them.

The "browser extensions" don't function properly unless you've also installed the 1Password app (https://agilebits.com/downloads), which is commercial (though there's a 30-day trial period).  If I get desperate enough I'll also install the app.
Here are two crashes in today's mozilla-central nightly:

bp-903e6177-2684-4bbf-9b50-606f72130808
bp-b6d0e3e8-0750-4f6b-b5b2-2dbc02130808

So apparently Masayuki's patch from comment #37 (attachment 778350 [details] [diff] [review]) doesn't stop these crashes.
> Here are two crashes in today's mozilla-central nightly:

Actually they're in yesterday's mozilla-central nightly (and in fact I mentioned both of them in comment #78).  But even this contains Masayuki's latest patch.
I regularly hit this crash on startup with my large profile (about 1000 tabs) and master password enabled. If the master password dialogs pops up very quickly after a restart and the session is restoring, when I got to type my master password I hit this crash. To avoid crashing I have to click cancel and wait for the dialog to pop up again and then I don't hit this crash. It seems about 25% of these crashes are within the first minute of uptime like mine are. I tried to reproduce this is a fresh profile with MP and restoring a tab with a password but I was unsuccessful. Since I can reproduce fairly easily in my main profile, I can test patches for you. Does that help at all?

bb8be88f-879a-470e-8640-4b6822140420
Flags: needinfo?(masayuki)
(In reply to Matthew N. [:MattN] from comment #81)
> I regularly hit this crash on startup with my large profile (about 1000
> tabs) and master password enabled. If the master password dialogs pops up
> very quickly after a restart and the session is restoring, when I got to
> type my master password I hit this crash. To avoid crashing I have to click
> cancel and wait for the dialog to pop up again and then I don't hit this
> crash. It seems about 25% of these crashes are within the first minute of
> uptime like mine are. I tried to reproduce this is a fresh profile with MP
> and restoring a tab with a password but I was unsuccessful. Since I can
> reproduce fairly easily in my main profile, I can test patches for you. Does
> that help at all?
> 
> bb8be88f-879a-470e-8640-4b6822140420

If I'm reading this right, this report seems to indicate that this is a deliberate crash due to "A password editor has focus, but not in secure input mode" at
http://hg.mozilla.org/mozilla-central/annotate/582b2d81ebe1/widget/cocoa/nsChildView.mm#l5426
Oh, you're the first person who can reproduce this bug without onepassword addon.

When you reproduce this crash, does the password field has caret and it is blinking? And input from keyboard normally? If so, we must have security issue in the edge case...
Flags: needinfo?(masayuki)
I am hitting this MOZ_CRASH today, and I don't have 1Password installed either.

STR:

1. Visit https://www.ubank.com.au/.
2. Click "Login" and wait for the login form to load.
3. Type something into the username and password inputs, leaving focus in the password input.
4. Press Enter then quickly afterwards press Cmd+T.
I can't seem to reproduce this in a non-e10s window.  Might just be because the timing of things changes, making it harder to get my keypress timing right though.
Actually step 4 doesn't need to use Cmd+T, I tried just pressing A after Enter, and that caused the crash too.  Between pressing Enter and the other key, the "Would you like to remember the password" doorhanger pops up.  Could that be related?
I tried logging in without triggering the crash, and then saved the password, so that the next time the remember password doorhanger wouldn't pop up.  Then I tried triggering the crash by logging in again and it didn't.
Note that there are two bugs for these crashes:

One (this bug) for non-e10s crashes.

Another (bug 1051842) for e10s crashes.
Summary: crash in -[ChildView keyDown:] → Non e10s crash in -[ChildView keyDown:]
This is still a regular on the nightly crash lists
Crash Signature: [@ -[ChildView keyDown:]] → [@ -[ChildView keyDown:]] [@ -[ChildView keyDown:] ]
Summary: Non e10s crash in -[ChildView keyDown:] → Non e10s crash in -[ChildView keyDown:] [Mac]
Whiteboard: [leave open] → [leave open][tbird crash]
Still can reproduce this bug on my Aurora and Nightly with e10s disabled.
This bug doesn't seem to affect Release and Beta for now.
Also, I found that not only 1Password but also another similar application can cause this crash too.
Perhaps any applications that show a floating window by hitting global shortcut keys and get focus
while you are entering into or giving focus in a password field, could cause this crash.

I tested on these two versions of Firefox with a new profile (i.e. no extensions installed):
- Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:40.0) Gecko/20100101 Firefox/40.0
- Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:41.0) Gecko/20100101 Firefox/41.0

STR:
https://crash-stats.mozilla.com/report/index/114df9e5-12b8-49f1-bb7b-b6b1b2150616
1. Install 1Password and launch 1Password mini.
2. Open a form with a password field, such as https://github.com/login
3. Focus in a password field.
4. Hit Command+Shift+\ to open the 1Password mini window.
5. Hit Enter to copy some information (such as password) on the mini window.
6. Crashed after the mini window is automatically closed.

STR2:
https://crash-stats.mozilla.com/report/index/3a288c87-64d2-4775-a305-5b23e2150616
1. Install ClipMenu, a clipboard manager, from http://www.clipmenu.com/
2,3. ditto.
4. Hit Command+Shift+V to pop up the ClipMenu's clipboard history.
5. Hit Enter to paste an item from the menu into the password field.
6. Crashed with nothing entered.
(In reply to Steven Michaud [:smichaud] from comment #91)
> Note that there are two bugs for these crashes:
> 
> One (this bug) for non-e10s crashes.
> 
> Another (bug 1051842) for e10s crashes.

FWIW I got an e10s crash (https://crash-stats.mozilla.com/report/index/c6ce78eb-44cf-44b2-9086-41a252150709) on today's nightly, but bug 1051842 is resolved fixed.
nodaguti, thanks for your STR!

I'm working on the 1Password bug at bug 1163339.
This bug has been fixed on the trunk (mozilla-central, currently the 43 branch).  See bug 1148196.  Its patch will shortly be uplifted to the 42 branch (aurora, Developer Edition).
Status: REOPENED → RESOLVED
Closed: 11 years ago → 9 years ago
Resolution: --- → FIXED
Whiteboard: [leave open][tbird crash] → [tbird crash]
Blocks: 1304709
See Also: → 1892562
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: