973344 - crash in nsTextEditorState::SetValue(nsAString_internal const&, bool, bool)

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Robert Kaiser]

This bug was filed from the Socorro interface and is 
report bp-1f48ba7c-1b3e-440b-bddf-2d6f22140215.
=============================================================

This is the top crash in 28.0b3 by far, happening on different Windows, Mac and Linux systems.

It's clearly a regression in beta 3 - there are a very few crashes with this signature in older Firefox versions, but nothing with that high a volume. Also, just in case you wonder, this does not have "AMD crash" characteristics.

We should check the checkins between beta 2 and 3 for what causes the regression.


Top Frames:

0 	xul.dll 	nsTextEditorState::SetValue(nsAString_internal const &,bool,bool) 	content/html/content/src/nsTextEditorState.cpp
1 	xul.dll 	mozilla::dom::HTMLInputElement::SetValueInternal(nsAString_internal const &,bool,bool) 	content/html/content/src/HTMLInputElement.cpp
2 	xul.dll 	mozilla::dom::HTMLInputElement::SetValue(nsAString_internal const &,mozilla::ErrorResult &) 	content/html/content/src/HTMLInputElement.cpp
3 	xul.dll 	mozilla::dom::HTMLInputElementBinding::set_value 	obj-firefox/dom/bindings/HTMLInputElementBinding.cpp
4 	xul.dll 	mozilla::dom::HTMLInputElementBinding::genericSetter 	obj-firefox/dom/bindings/HTMLInputElementBinding.cpp
5 	mozjs.dll 	js::Invoke(JSContext *,JS::Value const &,JS::Value const &,unsigned int,JS::Value *,JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
6 	xul.dll 	mozilla::dom::DOMProxyShadows(JSContext *,JS::Handle<JSObject *>,JS::Handle<int>) 	dom/bindings/DOMJSProxyHandler.cpp
7 	mozglue.dll 	arena_bin_nonfull_run_get 	memory/mozjemalloc/jemalloc.c
8 	mozjs.dll 	js::Shape::set(JSContext *,JS::Handle<JSObject *>,JS::Handle<JSObject *>,bool,JS::MutableHandle<JS::Value>) 	js/src/vm/Shape-inl.h
9 	mozjs.dll 	js::baseops::SetPropertyHelper<0>(JSContext *,JS::Handle<JSObject *>,JS::Handle<JSObject *>,JS::Handle<int>,unsigned int,JS::MutableHandle<JS::Value>,bool) 	js/src/jsobj.cpp
10 	mozjs.dll 	JSObject::setGeneric(JSContext *,JS::Handle<JSObject *>,JS::Handle<JSObject *>,JS::Handle<int>,JS::MutableHandle<JS::Value>,bool) 	js/src/jsobj.h
11 	mozjs.dll 	js::SetProperty<1>(JSContext *,JS::Handle<JSObject *>,JS::Handle<int>,JS::Value const &) 	js/src/vm/Interpreter.cpp
12 	mozjs.dll 	js::jit::DoSetPropFallback 	js/src/jit/BaselineIC.cpp
13 		@0x9259eb 	
14 		@0x921311 	
15 	mozjs.dll 	EnterBaseline 	js/src/jit/BaselineJIT.cpp
[...]

Comment 1

[Robert Kaiser]

Oh, find more crashes at https://crash-stats.mozilla.com/report/list?signature=nsTextEditorState%3A%3ASetValue%28nsAString_internal+const%26%2C+bool%2C+bool%29

Comment 2

[Robert Kaiser]

(In reply to Robert Kaiser (:[e-mail]) from comment #0)
> We should check the checkins between beta 2 and 3 for what causes the
> regression.

That makes this the regression range: http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX_28_0b2_RELEASE&tochange=FIREFOX_28_0b3_RELEASE

Comment 3

[Robert Kaiser]

I can't see anything obvious in the regression range, adding Benjamin and David, maybe they have an idea.

Comment 4

[(no longer active)]

This can only be related to bug 962313.  With <input type=number> disabled, we treat such inputs as <input type=text> which could somehow trigger this (although I don't have a good idea why.)

One plausible problem could be that we have something which is not hidden behind the dom.forms.number pref where it should be.  Assigning to jwatt since he knows the most about <input type=number>.

Comment 5

[Robert Kaiser]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #4)
> One plausible problem could be that we have something which is not hidden
> behind the dom.forms.number pref where it should be.

That was my only but vague suspicion as well when looking at the regression range on hg, but I wanted some dev to take a look, thanks for doing that!

Comment 6

[Sylvestre Ledru [:Sylvestre]]

As kairo said, I think that we should back re-enable "input=number" for the beta4.
Can someone prepare a commit ?

Comment 7

[(no longer active)]

(In reply to comment #6)
> As kairo said, I think that we should back re-enable "input=number" for the
> beta4.

That is almost certainly the wrong "fix" here, since <input type=number> is not ready to be shipped.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

I might be wrong but I have the feeling that we had less crashes with the feature enabled. 
Anyway, this would be enabled just for beta4 (which is supposed to be built today) and disabled for beta5.
And, for now, there is no bug fix.

Comment 9

[(no longer active)]

(In reply to comment #8)
> I might be wrong but I have the feeling that we had less crashes with the
> feature enabled. 

I can believe that, but the volume of crashes is not the only criterion based on which we choose whether we ship a feature.  :-)

> Anyway, this would be enabled just for beta4 (which is supposed to be built
> today) and disabled for beta5.
> And, for now, there is no bug fix.

I don't understand what enabling <input type=number> for beta4 will buy us in addition to masking this bug in beta4 (if my theory is correct, this *will* show up when we go to release without <input type=number>).

Please, let's give Jonathan time to investigate this before jumping to conclusions.  For all I know, my theory may be completely bogus.

Comment 10

[Jonathan Watt [:jwatt]]

Ehsan and I just spent some time digging into this, and Ehsan pointed out that some of the selectors in forms.css continue to apply even with the pref disabled. It turns out the editor is particularly fragile with regards to its assumptions about the frame tree that it's paired with, and these selectors cause frame tree changes that break those assumptions.

As it turns out the selectors that are causing the problem are old hacks for bug 946184 that can now be removed since that bug is fixed. There is still the issue of the |input[type=number]| selector which we're still discussing our preferred option of handling. Although that doesn't contribute to the crashes it seems like a bad idea to have it apply when the pref is disabled.

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: part 1 - remove the workarounds introduced by bug 947718 now that bug 946184 is fixed

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: part 2 for beta only - disable the input[type=number] forms.css rule, and the tests that break as a result

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: part 2 for beta only - disable the input[type=number] forms.css rule, and the tests that break as a result

Sorry, that was the aurora patch. This is the beta patch.

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8377329 [details] [diff] [review]
part 2 for beta only - disable the input[type=number] forms.css rule, and the tests that break as a result

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

r=me

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 8377261 [details] [diff] [review]
part 1 - remove the workarounds introduced by bug 947718 now that bug 946184 is fixed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 962313
User impact if declined: topcrash on beta
Testing completed (on m-c, etc.): none - beta only for part 2
Risk to taking this patch (and alternatives if risky): small risk, but almost no risk of something as bad as this topcrash
String or IDL/UUID changes made by this patch: none

We'll want this on aurora and m-c too. This code should now be redundant so should be okay on beta, but the failure to remove this workaround code earlier could mean it could potentially hide bugs (although I don't think that's the case).

This is the patch that really fixes the beta crash.

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 8377329 [details] [diff] [review]
part 2 for beta only - disable the input[type=number] forms.css rule, and the tests that break as a result

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 962313
User impact if declined: some risk of other v28 (pref off) issues we've not spotted yet
Testing completed (on m-c, etc.): none - beta only for part 2
Risk to taking this patch (and alternatives if risky): very little. expected to be more risky _not_ to take this
String or IDL/UUID changes made by this patch: none

Comment 17

[Robert Kaiser]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #9)
> (In reply to comment #8)
> > I might be wrong but I have the feeling that we had less crashes with the
> > feature enabled. 
> 
> I can believe that, but the volume of crashes is not the only criterion
> based on which we choose whether we ship a feature.  :-)

We have fix patches in here now, so the argument might be getting moot for this bug specifically, but just FTR, doubling the crash rate pretty surely makes people leave Beta, and we can't accept that. What I said in the request to release management (but didn't post here) was to back out the pref change and re-enable this temporarily so that we can ship a beta 4 that doesn't crash that much and make people leave this channel, with a proper fix and re-disabling planned to come for beta 5, hopefully. Nobody suggested we ship it in release because of the crashes, only to buy us some time to fix it properly as we can't accept such a high amount of crashes on the beta channel.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8377261 [details] [diff] [review]
part 1 - remove the workarounds introduced by bug 947718 now that bug 946184 is fixed

Let's get this landed to all branches, but approving for beta landing so we can go to build today, please simu-land in this instance.

Comment 19

[Jonathan Watt [:jwatt]]

Beta pushes (other branches coming up):

https://hg.mozilla.org/releases/mozilla-beta/rev/e8f98bacb0b2
http://hg.mozilla.org/releases/mozilla-beta/rev/f66b06faac65

kairo: FWIW I very much agree that minimizing any reduction in our beta pool is high priority, which is why I jumped right on this. I wasn't privy to the email about temporarily re-enabling the pref, and from comment 6 it wasn't apparent that the suggestion was as a temporary measure. FWIW I think doing that as a temporary measure is something we should have done.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

(In reply to Jonathan Watt [:jwatt] from comment #19)
> from comment 6 it wasn't apparent that the suggestion was as a temporary measure.
Sorry about the bad wording. When I said "for the beta4", I was implying that beta5 would have a better fix.
(cf comment #8)

Comment 21

[Jonathan Watt [:jwatt]]

Ah, I misread your comment 8. Sorry about that.

Comment 22

[u279076]

Flagging for verification once this is on Beta and we have some stats.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Calling 29/30 "affected" for tracking purposes (until part 1 lands on m-c/aurora).

Comment 24

[Robert Kaiser]

(In reply to Jonathan Watt [:jwatt] from comment #19)
> kairo: FWIW I very much agree that minimizing any reduction in our beta pool
> is high priority, which is why I jumped right on this.

Thanks for that for sure! And yes, I should have commented here what I emailed as well, I missed that because I was heading out when I wrote the email.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e8f98bacb0b2
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/f66b06faac65

Comment 26

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3c6daceb710
https://hg.mozilla.org/releases/mozilla-aurora/rev/a4b2ec56361d

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

This may be a duplicate, but I'm not sure: bug 973183.

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d3c6daceb710

Comment 30

[(no longer active)]

This is not completely fixed yet.

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks Ehsan. I was worried that it didn't seem to be fixed for pdfs.

Comment 32

[(no longer active)]

Oh, with bug 974430, we can probably close this.  jwatt?

Comment 33

[u279076]

0 crashes found with Nightly, Aurora, and Beta 4 in the last week. The most recent build with this signature is 28.0b3 (20140213172947):

https://crash-stats.mozilla.com/report/list?signature=nsTextEditorState%3A%3ASetValue%28nsAString_internal+const%26%2C+bool%2C+bool%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&version=Firefox%3A28.0b&hang_type=any&date=2014-02-24+18%3A00%3A00&range_value=1#tab-reports

As such I believe we can call this fixed. Ehsan, I will leave that call up to you since you reopened this bug.

Comment 34

[u279076]

Marking this verified fixed based on the following crash-stats:

Firefox 30.0a1: 0 crashes in the last week
Firefox 29.0a2: 0 crashes in the last week
Firefox 28.0b4: 17 crashes in the last week, all from one user
Firefox 28.0b3:	134131 crashes in the last week from 80846 users

Can this bug be closed now?

Comment 35

[u279076]

I'm calling this fixed based on crash-stats. There are no longer any crashes reported past Firefox 28.0b4.

Comment 36

[u279076]

This continues to drop off as users get updated to new Beta versions, down 34.41% in the last week.

Comment 37

[alex_mayorga]

Mr. Hughes,

Are you certain this is fixed?

My SO's Firefox 28 final went crash happy twice earlier today (please see below) and crash signatures point to this bug.

Same bug or do I file new one?

Submitted Crash Reports
Report ID              Date Submitted
bp-bc0ad0c9-86a7-43b3-9cd8-032e22140331         31/03/2014           11:44 a.m.
bp-67987a09-7d8c-400e-acfc-6056e2140331          31/03/2014           11:43 a.m.
bp-c3ad69a4-29b5-49c6-b4be-ba2502140331         31/03/2014           11:42 a.m.
bp-aab1314a-5bd7-40f6-ba36-c43642140331          31/03/2014           11:42 a.m.
bp-a5122453-eb26-45b7-ae1e-238912140331        31/03/2014           11:42 a.m.
bp-84108513-fb7c-4f59-8851-32b062140331           31/03/2014           11:41 a.m.
bp-159010b0-85b8-468e-a48f-4cb042140331          31/03/2014           11:41 a.m.
bp-c7c88a83-e0c6-4ea0-bcba-93ddd2140331         31/03/2014           11:40 a.m.
bp-d0ef96f5-3e3b-4eb0-8d76-624332140331           31/03/2014           11:39 a.m.
bp-901d2ccd-e01f-48b8-8e5f-dd69f2140331             31/03/2014           11:39 a.m.
bp-83f8f58f-e287-44c2-936b-9f9e02140331              31/03/2014           11:39 a.m.
bp-e5661c2d-8de2-4ae3-a06c-6464d2140331         31/03/2014           11:39 a.m.
bp-63bcf7f1-1058-45fb-beeb-b09a12140331            31/03/2014           11:38 a.m.
bp-95f29c60-803d-4736-964e-c7a6b2140331          31/03/2014           11:38 a.m.
bp-2cdbf8e7-aeda-405f-b3fd-e554e2140331            31/03/2014           11:38 a.m.
bp-aaa225d2-381b-4c66-a749-9c4eb2140331         31/03/2014           11:37 a.m.
bp-d93a8d89-b209-406a-8c8a-da9fd2140331          31/03/2014           11:37 a.m.
bp-6b182401-4dda-4c9d-b859-9194f2140331          31/03/2014           11:36 a.m.
bp-4d0792d4-15ea-4ec4-8cd6-68a752140331         31/03/2014           11:36 a.m.
bp-4f75c299-0016-46dc-8e92-cd2f42140331            31/03/2014           11:35 a.m.
bp-51ab2a2d-e4a1-4270-833b-b529e2140331        31/03/2014           11:34 a.m.
bp-54c91413-41de-4e82-883b-45a4b2140331        31/03/2014           11:33 a.m.
bp-53c0c2f8-6c0f-4839-a6bc-8e1572140331            31/03/2014           11:32 a.m.
bp-b594e9b9-48fb-43d3-aaf9-0663a2140331           31/03/2014           11:32 a.m.
bp-45e095b0-0816-4d6c-a7e8-034e62140331        31/03/2014           11:31 a.m.
bp-1f1a7d65-0e2a-4152-a3df-161692140331           31/03/2014           11:30 a.m.
bp-5bf22640-0b0b-41ec-b653-e7e2c2140331          31/03/2014           10:05 a.m.
bp-494239bd-8631-4842-a51a-d68b52140331        31/03/2014           10:05 a.m.
bp-d8dd8064-ef8f-46be-ad10-a11872140331           31/03/2014           10:04 a.m.
bp-35b7336f-0d3c-44fe-9829-4054b2140331           31/03/2014           10:04 a.m.
bp-d057f301-978c-476b-b941-478192140331          31/03/2014           10:04 a.m.

Comment 38

[Benjamin Smedberg]

alex, as noted in the target milestone this is fixed for Firefox 30, not Firefox 28.

Comment 39

[alex_mayorga]

Mr. Smedberg,

Thanks on educating me on the interpretation of "Target Milestone" field.

Any way to make it so it is 29 instead of 30?

Comment 40

[u279076]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #38)
> alex, as noted in the target milestone this is fixed for Firefox 30, not
> Firefox 28.

Actually, this was fixed in Firefox 30 Nightly but uplifted to Firefox 29 Aurora and 28 Beta. Alex, looking at your crash reports you're getting a different stack. While the signature is the same I think this is a different bug.

Comment 41

[alex_mayorga]

Mr. Hughes,

Thanks! Filed https://bugzilla.mozilla.org/show_bug.cgi?id=991295 FWIW.

Please let me know if there's anything I shall provide.