749964 - crash in nsWebBrowserPersist::SaveDocumentInternal

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Scoobidiver (away)]

It first appeared in 15.0a1/20120427. The regression range might be:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cc5254f9825f&tochange=450d8cd16316

Signature 	nsWebBrowserPersist::SaveDocumentInternal(nsIDOMDocument*, nsIURI*, nsIURI*) More Reports Search
UUID	71515b0e-7192-4f92-8dd8-c1d6a2120428
Date Processed	2012-04-28 03:54:40
Uptime	51
Last Crash	4.5 weeks before submission
Install Age	9.5 hours since version was first installed.
Install Time	2012-04-27 18:26:05
Product	Firefox
Version	15.0a1
Build ID	20120427030500
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 42 stepping 7
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x38
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x0116, AdapterSubsysID: 1670103c, AdapterDriverVersion: 8.830.6.3000
D3D10 Layers? D3D10 Layers- D3D9 Layers? D3D9 Layers+ 
EMCheckCompatibility	True
Total Virtual Memory	4294836224
Available Virtual Memory	3880538112
System Memory Use Percentage	41
Available Page File	5484793856
Available Physical Memory	2487209984

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsWebBrowserPersist::SaveDocumentInternal 	embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp:1538
1 	xul.dll 	nsWebBrowserPersist::SaveDocument 	embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp:475
2 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
3 	xul.dll 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2408
4 	xul.dll 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1549
5 	mozjs.dll 	js::GetPropertyOperation 	js/src/jsinterpinlines.h:266
6 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2757
7 	mozjs.dll 	JSScript::makeAnalysis 	js/src/jsinfer.cpp:5372
8 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:475
9 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:535
10 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:567
11 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5416
12 	xul.dll 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:1889
13 	xul.dll 	nsJSEventListener::HandleEvent 	dom/src/events/nsJSEventListener.cpp:225
14 	xul.dll 	nsScriptSecurityManager::GetPrincipalAndFrame 	caps/src/nsScriptSecurityManager.cpp:2394
15 	xul.dll 	nsTArray<XPCJSContextInfo,nsTArrayInfallibleAllocator>::AppendElements<JSContext 	obj-firefox/dist/include/nsTArray.h:899
16 	xul.dll 	XPCJSContextStack::Push 	js/xpconnect/src/XPCThreadContext.cpp:128
17 	xul.dll 	nsCxPusher::Push 	content/base/src/nsContentUtils.cpp:2935
18 	xul.dll 	nsCxPusher::RePush 	content/base/src/nsContentUtils.cpp:2963
19 	xul.dll 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:349
20 	xul.dll 	nsWindowRoot::PreHandleEvent 	dom/base/nsWindowRoot.cpp:194
21 	mozjs.dll 	JS_FrameIterator 	js/src/jsdbgapi.cpp:507
22 	xul.dll 	nsEventDispatcher::DispatchDOMEvent 	content/events/src/nsEventDispatcher.cpp:747
23 	xul.dll 	nsINode::DispatchEvent 	content/base/src/nsGenericElement.cpp:1165
24 	xul.dll 	nsContentUtils::DispatchXULCommand 	content/base/src/nsContentUtils.cpp:5844

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsWebBrowserPersist%3A%3ASaveDocumentInternal%28nsIDOMDocument*%2C+nsIURI*%2C+nsIURI*%29

Comment 1

[[Baboo]]

I believe this can be triggered by trying to save a pop-up that is about to be closed just in the right moment, i.e. a race condition, sadly I had no luck reproducing it another time yet (tried about 10 times).

My original crash was bp-4d7f1e6c-04eb-4ef8-ae40-6ba3e2120820.

Comment 2

[[Baboo]]

Oh, now I got it, the pop-up needs to open in a tab, e.g. because you have browser.link.open_newwindow.restriction set to 0. Now I can reliable reproduce it on a clean profile: bp-1780b256-2332-4829-91e5-f5b1e2120820

The save dialog does not close when the tab gets automatically closed, unlike in the pop-up case, so making it close in the tab case too would be a good fix? Good candidate for a first patch?

Comment 3

[[Baboo]]

Attachment: 2nd part, do no open this one directly, it needs to be opened from a script or it can't close itself

Comment 4

[[Baboo]]

Attachment: 1st part, open this

Open this, let the "pop-up" open (but as a tab by having the appropriate about:config entries set, like browser.link.open_newwindow.restriction set to 0), quickly switch to it and do a "Save Page As…" before it closes, and click OK in the "Save As" file-picker dialog only after the tab has closed itself.

Comment 5

[[Baboo]]

Comment on attachment 653338 [details]
1st part, open this

Huh? Is this some sort of security feature that it will ignore me when I explicitly set the MIME type to text/html the first time?

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20121005042010

I tried to download attachment 668421 [details] and hit the same crash:

bp-37f765eb-d2e3-44ca-969a-0346e2121005

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

It's reproducible all the time with the above mentioned attachment. Marcia, would you mind to check how often this crash occurs at the moment?

Comment 8

[Boris Zbarsky [:bzbarsky]]

What happens here is that the "aDocument" argument passed to nsWebBrowserPersist::SaveDocument is some random non-document JS object.  XPConnect happily makes up an nsIDOMDocument, but then we QI it to nsIDocument, get null, and crash.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, looks like the JSObject passed in is a proxy.  The proxy handler is js::DeadObjectProxy.  So this is actually sort of fallout from bug 695480, which is why it appears in 15.

I'll add the null-check here, but I wonder whether we should refuse to create XPCWrappedJS around js::DeadObjectProxy instances!

Comment 10

[Boris Zbarsky [:bzbarsky]]

Note that this will cause the saveDocument calls to throw, by the way...

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Boris Zbarsky (:bz) from comment #9)
> I'll add the null-check here, but I wonder whether we should refuse to
> create XPCWrappedJS around js::DeadObjectProxy instances!

Probably.  We should probably also make nsIDOMDocument builtinclass.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yes, I was assuming there was some deep reason why it wasn't yet....

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

There might be!  Node isn't builtinclass either.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't try to save if what we have is not a real document object.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Marcia, I hope you don't mind me stealing this.  ;)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 668714 [details] [diff] [review]
Don't try to save if what we have is not a real document object.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): hueyfix
User impact if declined: Random crashes when trying to save self-closing moles,
   er, windows.
Testing completed (on m-c, etc.): Patched build doesn't crash on the testcases in
   this bug.  Instead it shows an alert that it could not save the document.
Risk to taking this patch (and alternatives if risky): Low risk.  Adds a
   null-check and exception instead of dereferencing null and crashing.
String or UUID changes made by this patch: None.

Comment 17

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/mozilla-central/rev/1c64ccca6860

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Rank is #207 in the topcrasher list for Firefox 15 and #158 for Firefox 16.0b6. Is it something we could check with a crash test or is it not worth the effort?

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 668714 [details] [diff] [review]
Don't try to save if what we have is not a real document object.

[Triage Comment]
Approving for Aurora 17 given this is just a null check fix for a low volume crasher. Please land early Monday PT to make the merge.

Comment 20

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1ae019103173

Alex, is this wontfix for 16 then?

Comment 21

[Alex Keybl [:akeybl]]

(In reply to Boris Zbarsky (:bz) from comment #20)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/1ae019103173
> 
> Alex, is this wontfix for 16 then?

Yep - updated the status flag.

Comment 22

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0

http://bit.ly/WzbcSp

No crashes in 17beta1 and a nice error message is displayed when saving the testcases which previously crashed the browser in F16.

Comment 23

[u279076]

Virgil, please keep the verifyme keyword on this bug until it's been verified across all fixed branches.

Comment 24

[Paul Silaghi, QA [:pauly]]

(In reply to Virgil Dicu [:virgil] [QA] from comment #22)
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0
> 
> http://bit.ly/WzbcSp
> 
> No crashes in 17beta1 and a nice error message is displayed when saving the
> testcases which previously crashed the browser in F16.

Verified fixed on  Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0b1