1328566 - Crash [@ OOM | large | NS_ABORT_OOM | AppendASCIItoUTF16 | mozilla::dom::NodeBinding::get_nodeValue ]

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect, P3)

Description

[Carsten Book [:Tomcat]]

found via bughunter and reproduced with latest beta opt tinderbox build.

affects trunk -> beta opt builds

Steps to reproduce
-> Load http://www.tallysolutions.com/tallyweb/modules/pss/crm/kb/op/CKBContentPreviewMgr.php?strProductFlag=0&show_content_flag=1&content_id=3564&scope_id=12&strKbAdminFlag=0&strProductId=5&strInvokedFromSupportCentreFlag=0&strSCIframeName=&strInvokationMode=0&strAccID=-4&strSrNum=0&click_journal_flag=1&curr_ctt_tgt=3&keyword=T-FV-2063%2BValid%2BCount%2Bof%2BChallan/Transfer%2BVoucher%2BRecords%2Bmust%2Bbe%2Bprovided&search_type=2

--> Crash after 10-30 seconds 

Crash-ID: https://crash-stats.mozilla.com/report/index/6859bbaa-7236-4e5a-b0b2-27d0b2170104

Crash Report [@ OOM | large | NS_ABORT_OOM | AppendASCIItoUTF16 | mozilla::dom::NodeBinding::get_nodeValue ]

Comment 1

[Carsten Book [:Tomcat]]

[Tracking Requested - why for this release]:

Opt Crash/OOM found by bughunter 

Eric, could you take a look at this, thanks!

Comment 2

[Benjamin Smedberg]

This is DOM, not XPCOM. If we intend to prevent this kind of OOM the DOM code needs to use fallible string APIs.

Comment 3

[Andrew McCreight [:mccr8]]

FWIW, this does not look very common. I only see about 30 crashes across all versions.

Comment 4

[Eric Rahm [:erahm]]

If we wanted to make that operation fallible we'd have to make nsINode::GetNodeValue [1] return an error, the generated dom binding side would need to propagate an exception, and then all the implementations of GetNodeValueInternal would have to switch over to fallible string conversions.

Just checking out that page in my 64-bit Linux build destroys browser performance, so it's clearly doing bad things but reproducing the OOM will be hard do to the size. Loading in a 32-bit build would probably reproduce easily.

[1] http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/dom/base/nsINode.h#1780

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I wonder how bad it would be to return empty string in this case. Not correct sure, but at least wouldn't crash.
Throwing exception from .nodeValue isn't really right either, per spec.

But where are we crashing exactly? Are we perhaps doing some dummy memory copy?

Comment 6

[Gerry Chang [:gchang]]

Track 51- as the volume of crash is low.

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

NI John to take a look, thanks!

Comment 8

[John Dai[:jdai]]

Keep NI for me, I'll take a look.

Comment 9

[Julien Cristau [:jcristau]]

Not a huge volume, wontfix for 51.

Comment 10

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

It would be nice to get a fix here but there are only 10 crashes on release with this signature.... It doesn't seem useful for relman to track this. If anyone lands a patch please feel free to request uplift as far as you think makes sense.

Comment 11

[Julien Cristau [:jcristau]]

Too late for firefox 52, mass-wontfix.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.