1113121 - Godaddy webmail crash [EXCEPTION_ACCESS_VIOLATION_READ]

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Toady]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141126041045

Steps to reproduce:

Replied to email, Pasted small block of text from windows clipboard then
tried to delete a typo pressed backspace key 2 times.


Actual results:

Browser closed to desktop showing crash reporter.
https://crash-stats.mozilla.com/report/index/bp-4371a75f-4e2d-4fd5-bc1f-dd8ee2141217


Expected results:

Browser should not have crashed.

Unfortunately i have not been able to reproduce this issue other then the initial, However if i can i will post more information.

There seems to be others with the same signature, https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=nsINode%3A%3ALength%28%29#tab-comments

Possibly caused by anti-virus

Comment 1

[:Gijs (out for now; he/him)]

This stack looks like editor...

Comment 2

[:Gijs (out for now; he/him)]

Ehsan - editor crashiness with identical stacks leading to nsINode::Length() (and implicitly, NodeType() - so looks like mNodeInfo is dead? Maybe I'm misreading the code...) any idea what's going on here? It looks like this started in 34, so that'd tie up nicely with bug 1055032... but I don't know how much change there is in editor-land and if that's a reasonable suspicion.

(confirming per multiple reports)

Comment 3

[Toady]

Was able to produce it on firefox 35.0b8 https://crash-stats.mozilla.com/report/index/bp-7611bc2f-4529-4f5e-9eec-432c62150110

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I suspect that a debug build will trigger the MOZ_ASSERT(aParent) line. nsHTMLEditRules::JoinNodesSmart will need a null check.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

And the regression was in mozilla-central changeset 4439b62ad770; before, JoinNodesSmart would call the variant of MoveNode that takes nsIDOMNode and checks for null, rather than the variant that takes nsINode and asserts.

Comment 6

[(no longer active)]

[Tracking Requested - why for this release]: Regression from bug 1055032 which landed in Firefox 34.  This can cause Firefox to crash.

Comment 7

[(no longer active)]

Attachment: Null check the parent node in nsHTMLEditRules::JoinNodesSmart() before passing it to MoveNode

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8547679 [details] [diff] [review]
Null check the parent node in nsHTMLEditRules::JoinNodesSmart() before passing it to MoveNode

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

Test?

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I believe the previous code might have early-returned, fwiw.

Comment 10

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> Comment on attachment 8547679 [details] [diff] [review]
> Null check the parent node in nsHTMLEditRules::JoinNodesSmart() before
> passing it to MoveNode
> 
> Review of attachment 8547679 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Test?

I'm not sure how to reproduce the bug...

Toady, can you please give me some information on how to trigger the crash?  Specifically, what do you exactly copy to the clipboard in comment 0?  Thanks!

Comment 11

[(no longer active)]

(In reply to :Ms2ger from comment #9)
> I believe the previous code might have early-returned, fwiw.

Yes, you're right <https://hg.mozilla.org/mozilla-central/rev/4439b62ad770#l1.18>.  I'll land the patch as an early return to keep the same behavior, although I'm not sure if it would make a difference either way.

Comment 12

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1212b70ab9d9

Comment 13

[(no longer active)]

Attachment: Null check the parent node in nsHTMLEditRules::JoinNodesSmart() before passing it to MoveNode

Comment 14

[Nigel Babu [:nigelb]]

This causes a build bustage https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5380398&repo=mozilla-inbound

Comment 15

[:Benjamin Peterson]

Fix assertion: https://hg.mozilla.org/integration/mozilla-inbound/rev/7e143e1f0852

Comment 16

[(no longer active)]

Ouch, sorry, and thanks for the fix!

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1212b70ab9d9
https://hg.mozilla.org/mozilla-central/rev/7e143e1f0852

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

wontfix for 34 as 35 has already been released.

Ehsan - Please request uplift once you think this patch has been proven good on Nightly. Do you think this is severe enough to consider as a ride along for 35?

Comment 19

[Toady]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> > Comment on attachment 8547679 [details] [diff] [review]
> > Null check the parent node in nsHTMLEditRules::JoinNodesSmart() before
> > passing it to MoveNode
> > 
> > Review of attachment 8547679 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Test?
> 
> I'm not sure how to reproduce the bug...
> 
> Toady, can you please give me some information on how to trigger the crash? 
> Specifically, what do you exactly copy to the clipboard in comment 0? 
> Thanks!

So far i have only been able to trigger the crash 2 times one in 34.0.5 and one in 35.0 the exact steps to produce it are not exact and are inconsistent so not easily reproducible.

But the last crash in 35.0

I basically just typed text then backspaced it out, Then i would copy and paste a block of text into the form and backspace it out and repeat the above, Then out of the blue you press the backspace escaping 2 characters then the crash occurred.

The first crash i was composing an email and pasted in some text then went to backspace out a typo then Firefox closed and the crash reporter appeared, The second in 35.0 i basically have been trying everyday to narrow down the exact steps but sorry its far to inconsistent.

I have seen this message in the browser console "Security Error: Content at https://email19.asia.secureserver.net/pcompose.php may not load or link to chrome://starfieldzoom/content/logo.png." 

If there is a test build i can try i will see if i can reproduce it.

Comment 20

[(no longer active)]

Thanks for the information, Toady.  The fix has been added to Nightly, which is an unofficial Firefox build built every night from the most recent code.  You can download it from <http://nightly.mozilla.org/>.  It would be great if you can test to see if you can get a crash running that build.  If you can, please include a link to the crash report on Nightly.  Thanks again!

Comment 21

[(no longer active)]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #18)
> wontfix for 34 as 35 has already been released.
> 
> Ehsan - Please request uplift once you think this patch has been proven good
> on Nightly. Do you think this is severe enough to consider as a ride along
> for 35?

Given how safe the fix is, I think we should consider this as a ride-along for 35 if we do a dot release.  But I would like to wait for Toady to test Nightly given that we don't have an automated test for this...

Comment 22

[Toady]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #21)
> (In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #18)
> > wontfix for 34 as 35 has already been released.
> > 
> > Ehsan - Please request uplift once you think this patch has been proven good
> > on Nightly. Do you think this is severe enough to consider as a ride along
> > for 35?
> 
> Given how safe the fix is, I think we should consider this as a ride-along
> for 35 if we do a dot release.  But I would like to wait for Toady to test
> Nightly given that we don't have an automated test for this...

I have tried for a total of 3hrs to reproduce it in nightly an are unable to produce the crash, So i would say you nailed it.

Vendor=Mozilla
Name=Firefox
RemotingName=firefox
CodeName=Nightly
Version=38.0a1
BuildID=20150115030228
SourceRepository=https://hg.mozilla.org/mozilla-central
SourceStamp=c1f6345f2803

Comment 23

[(no longer active)]

Great, thanks a lot for the bug report and for helping us verify the fix!

Comment 24

[(no longer active)]

Attachment: Branch patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1055032.
[User impact if declined]: Crash when using software such as webmails, blog editing UI, wikipedia editing UI, etc.
[Describe test coverage new/current, TBPL]: We don't have automated tests for the fix since we don't know the exact content that triggers the bad behavior, but the crash is very well understood and the fix has been manually verified on trunk.
[Risks and why]: This is as low risk as it gets (it merely adds a null check for a pointer that will be dereferenced immediately after.)  I think it's safe to even take as a dot release ride along.
[String/UUID change made/needed]: None.

Comment 25

[(no longer active)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #21)
> (In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #18)
> > wontfix for 34 as 35 has already been released.
> > 
> > Ehsan - Please request uplift once you think this patch has been proven good
> > on Nightly. Do you think this is severe enough to consider as a ride along
> > for 35?
> 
> Given how safe the fix is, I think we should consider this as a ride-along
> for 35 if we do a dot release.  But I would like to wait for Toady to test
> Nightly given that we don't have an automated test for this...

Needinfoing Lawrence since this may be time critical.

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

Transferring my ni to lsblakk, who is managing 35.

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking and adding to the list of potential ride-along fixes if we need to do a chemspill or dot release.

Comment 28

[(no longer active)]

We should track this for 38 as well.

https://hg.mozilla.org/releases/mozilla-aurora/rev/486b783c09b6
https://hg.mozilla.org/releases/mozilla-beta/rev/64d25509541e

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8550290 [details] [diff] [review]
Branch patch

We will probably do a 35.0.1. Taking it.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/1f81a371dba5

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-release/rev/5bf4aaad73fe

https://treeherder.mozilla.org/logviewer.html#?job_id=34738&repo=mozilla-release

Comment 32

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-release/rev/74760b4224b0

Comment 33

[Bogdan Maris, Desktop Test Engineering]

Toady, can you please try and see if this issue does reproduce for you in Firefox 35.0.1 build? I am unable to make a Godaddy webmail account.
Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/35.0.1-candidates/build1/win32/en-US/firefox-35.0.1.zip

Comment 34

[Toady]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #33)
> Toady, can you please try and see if this issue does reproduce for you in
> Firefox 35.0.1 build? I am unable to make a Godaddy webmail account.
> Build:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/35.0.1-candidates/
> build1/win32/en-US/firefox-35.0.1.zip

Hello, Yes unable to produce the issue in 35.0.1 the issue appears to be resovled :)
Vendor=Mozilla
Name=Firefox
RemotingName=firefox
Version=35.0.1
BuildID=20150122214805
SourceRepository=https://hg.mozilla.org/releases/mozilla-release
SourceStamp=5ea9473a99ff

Comment 35

[Bogdan Maris, Desktop Test Engineering]

(In reply to Toady from comment #34)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #33)
> > Toady, can you please try and see if this issue does reproduce for you in
> > Firefox 35.0.1 build? I am unable to make a Godaddy webmail account.
> > Build:
> > http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/35.0.1-candidates/
> > build1/win32/en-US/firefox-35.0.1.zip
> 
> Hello, Yes unable to produce the issue in 35.0.1 the issue appears to be
> resovled :)
> Vendor=Mozilla
> Name=Firefox
> RemotingName=firefox
> Version=35.0.1
> BuildID=20150122214805
> SourceRepository=https://hg.mozilla.org/releases/mozilla-release
> SourceStamp=5ea9473a99ff

Awsome, thanks Toady! Based on your verification from comment 22 and comment 34 I will mark this as verified. Would you be so kind to verify this on latest Aurora and beta as well? Just to finally close this bug.

Beta build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/36.0b3-candidates/build1/win32/en-US/firefox-36.0b3.zip
Aurora build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora/firefox-37.0a2.en-US.win32.zip

Comment 36

[Toady]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #35)
> (In reply to Toady from comment #34)
> > (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #33)
> > > Toady, can you please try and see if this issue does reproduce for you in
> > > Firefox 35.0.1 build? I am unable to make a Godaddy webmail account.
> > > Build:
> > > http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/35.0.1-candidates/
> > > build1/win32/en-US/firefox-35.0.1.zip
> > 
> > Hello, Yes unable to produce the issue in 35.0.1 the issue appears to be
> > resovled :)
> > Vendor=Mozilla
> > Name=Firefox
> > RemotingName=firefox
> > Version=35.0.1
> > BuildID=20150122214805
> > SourceRepository=https://hg.mozilla.org/releases/mozilla-release
> > SourceStamp=5ea9473a99ff
> 
> Awsome, thanks Toady! Based on your verification from comment 22 and comment
> 34 I will mark this as verified. Would you be so kind to verify this on
> latest Aurora and beta as well? Just to finally close this bug.
> 
> Beta build:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/36.0b3-candidates/
> build1/win32/en-US/firefox-36.0b3.zip
> Aurora build:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora/
> firefox-37.0a2.en-US.win32.zip

Hello, Yes the issue is not present in Beta build & Aurora build you supplied.

The patches by Ehsan & Benjamin has resolved this issue. 

If you need me to run more tests please feel free to ask :)