Closed
Bug 893973
Opened 11 years ago
Closed 9 years ago
Non e10s crash in -[ChildView keyDown:] [Mac]
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
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)
2.72 KB,
patch
|
smichaud
:
review+
|
Details | Diff | Splinter Review |
3.57 KB,
patch
|
smichaud
:
review+
|
Details | Diff | Splinter Review |
3.44 KB,
patch
|
smichaud
:
review+
|
Details | Diff | Splinter Review |
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]
Comment 1•11 years ago
|
||
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.
Comment 2•11 years ago
|
||
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)
Comment 3•11 years ago
|
||
> 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)
Comment 4•11 years ago
|
||
> 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.)
Comment 5•11 years ago
|
||
> This bug's crashes are currently the #7 topcrasher on the 25 branch.
I should have clarified that this among Mac crashes.
Comment 6•11 years ago
|
||
> 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.
Comment 7•11 years ago
|
||
> 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?
Comment 8•11 years ago
|
||
> 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.
Comment 9•11 years ago
|
||
> 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.
Assignee | ||
Comment 10•11 years ago
|
||
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...
Assignee | ||
Comment 11•11 years ago
|
||
(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.
Assignee | ||
Comment 12•11 years ago
|
||
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.
Comment 13•11 years ago
|
||
> 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 14•11 years ago
|
||
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+
Assignee | ||
Comment 15•11 years ago
|
||
(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).
Assignee | ||
Comment 16•11 years ago
|
||
(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.
Assignee | ||
Comment 17•11 years ago
|
||
Comment 18•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment 19•11 years ago
|
||
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.
Comment 20•11 years ago
|
||
> 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.
Comment 21•11 years ago
|
||
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.
Comment 22•11 years ago
|
||
> 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.
Comment 23•11 years ago
|
||
> 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.
Comment 24•11 years ago
|
||
> 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).
Reporter | ||
Updated•11 years ago
|
Reporter | ||
Comment 25•11 years ago
|
||
It's not fixed. See bp-eee2bd70-d9b1-4e15-aac9-78dd82130718 with a slightly different stack trace.
Comment 26•11 years ago
|
||
> 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.
Comment 27•11 years ago
|
||
>> 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.
Assignee | ||
Comment 28•11 years ago
|
||
Hmm, but the crash indicates security bug since password file has focus without secure event input mode. So, I really want the STR...
Comment 29•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #778014 -
Attachment is patch: true
Comment 30•11 years ago
|
||
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.
Comment 31•11 years ago
|
||
(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).
Reporter | ||
Updated•11 years ago
|
Whiteboard: [leave open]
Reporter | ||
Comment 32•11 years ago
|
||
(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.
Comment 33•11 years ago
|
||
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.
Comment 34•11 years ago
|
||
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 35•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #778014 -
Flags: review?(ted) → review+
Assignee | ||
Comment 36•11 years ago
|
||
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+
Assignee | ||
Comment 37•11 years ago
|
||
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
Comment 38•11 years ago
|
||
Comment on attachment 778014 [details] [diff] [review]
Additional debugging patch
Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01d5bde15738
Comment 39•11 years ago
|
||
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+
Comment 40•11 years ago
|
||
Comment 41•11 years ago
|
||
Weird. It built fine on my system.
I do a tryserver build before submitting it again.
Comment 42•11 years ago
|
||
Relanded with minor changes on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05fd9daf1492
These changes worked find on tryserver.
Comment 43•11 years ago
|
||
Carrying over Ted's and Masayuki's r+s.
Attachment #778543 -
Attachment is obsolete: true
Attachment #778672 -
Flags: review+
Assignee | ||
Comment 44•11 years ago
|
||
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)
Comment 45•11 years ago
|
||
Reporter | ||
Comment 46•11 years ago
|
||
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 47•11 years ago
|
||
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+
Comment 48•11 years ago
|
||
(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.
Comment 49•11 years ago
|
||
Masayuki, let me know if you can think of any other information we could log that might be useful.
Attachment #782633 -
Flags: review?(masayuki)
Assignee | ||
Comment 50•11 years ago
|
||
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+
Comment 51•11 years ago
|
||
Comment on attachment 782633 [details] [diff] [review]
Add more debug info
Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/197b12188f3a
Comment 52•11 years ago
|
||
Comment 53•11 years ago
|
||
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.
Comment 54•11 years ago
|
||
Comment on attachment 782633 [details] [diff] [review]
Add more debug info
https://hg.mozilla.org/mozilla-central/rev/fd03bb4d1e48
Comment 55•11 years ago
|
||
(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
Assignee | ||
Comment 56•11 years ago
|
||
Hmm, it's very strange. I don't have any trouble on 10.7. Is it a bug of 10.8?
Assignee | ||
Comment 57•11 years ago
|
||
I installed 10.8, but I cannot reproduce the bug on my environment, why???
Comment 58•11 years ago
|
||
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.)
Comment 59•11 years ago
|
||
I'm on 10.8.4, FWIW.
Comment 60•11 years ago
|
||
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.
Comment 61•11 years ago
|
||
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)
Assignee | ||
Comment 62•11 years ago
|
||
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+
Comment 63•11 years ago
|
||
Comment on attachment 784632 [details] [diff] [review]
Add more debug info, rev1
Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ca804ec7136
Comment 64•11 years ago
|
||
Comment 65•11 years ago
|
||
(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!
Comment 66•11 years ago
|
||
> 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.
Comment 67•11 years ago
|
||
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.
Comment 68•11 years ago
|
||
(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!
Comment 69•11 years ago
|
||
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.
Comment 70•11 years ago
|
||
"please to think" -> "please try to think" :-)
Assignee | ||
Comment 71•11 years ago
|
||
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).
Assignee | ||
Comment 72•11 years ago
|
||
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.
Comment 73•11 years ago
|
||
> I'm guessing that attachment 778350 [details] [diff] [review] will _prevent_ this crash.
Go ahead and land it, and give it a try.
Assignee | ||
Comment 74•11 years ago
|
||
Comment 75•11 years ago
|
||
Assignee | ||
Comment 76•11 years ago
|
||
Good hint comes!!
https://crash-stats.mozilla.com/report/index/5bc59052-e132-4f09-9be6-35ac92130807
> Tried to use 1Password addon hotkey
Comment 77•11 years ago
|
||
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.
Comment 78•11 years ago
|
||
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.
Comment 79•11 years ago
|
||
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.
Comment 80•11 years ago
|
||
> 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.
Comment 81•11 years ago
|
||
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)
Comment 82•11 years ago
|
||
(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
Assignee | ||
Comment 83•11 years ago
|
||
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)
Comment 87•10 years ago
|
||
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.
Comment 88•10 years ago
|
||
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.
Comment 89•10 years ago
|
||
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?
Comment 90•10 years ago
|
||
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.
Comment 91•10 years ago
|
||
Note that there are two bugs for these crashes:
One (this bug) for non-e10s crashes.
Another (bug 1051842) for e10s crashes.
Updated•10 years ago
|
Summary: crash in -[ChildView keyDown:] → Non e10s crash in -[ChildView keyDown:]
Updated•10 years ago
|
tracking-e10s:
--- → -
Comment 92•10 years ago
|
||
This is still a regular on the nightly crash lists
Crash Signature: [@ -[ChildView keyDown:]] → [@ -[ChildView keyDown:]]
[@ -[ChildView keyDown:] ]
Updated•10 years ago
|
Summary: Non e10s crash in -[ChildView keyDown:] → Non e10s crash in -[ChildView keyDown:] [Mac]
Whiteboard: [leave open] → [leave open][tbird crash]
Comment 95•9 years ago
|
||
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.
Comment 96•9 years ago
|
||
(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.
Comment 97•9 years ago
|
||
nodaguti, thanks for your STR!
I'm working on the 1Password bug at bug 1163339.
Comment 99•9 years ago
|
||
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]
You need to log in
before you can comment on or make changes to this bug.
Description
•