Closed Bug 749964 Opened 8 years ago Closed 7 years ago

crash in nsWebBrowserPersist::SaveDocumentInternal

Categories

(Core Graveyard :: Embedding: APIs, defect, critical)

15 Branch
defect
Not set
critical

Tracking

(firefox15 wontfix, firefox16- wontfix, firefox17- verified, firefox18 verified)

VERIFIED FIXED
mozilla18
Tracking Status
firefox15 --- wontfix
firefox16 - wontfix
firefox17 - verified
firefox18 --- verified

People

(Reporter: scoobidiver, Assigned: bzbarsky)

References

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(3 files)

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
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.
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?
Attachment #653335 - Attachment mime type: text/plain → text/html
Attached file 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 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?
Attachment #653338 - Attachment mime type: text/plain → text/html
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
OS: Windows 7 → All
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?
Assignee: nobody → mozillamarcia.knous
Keywords: qawanted
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.
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!
Blocks: hueyfix
Note that this will cause the saveDocument calls to throw, by the way...
(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.
Yes, I was assuming there was some deep reason why it wasn't yet....
There might be!  Node isn't builtinclass either.
Marcia, I hope you don't mind me stealing this.  ;)
Assignee: mozillamarcia.knous → bzbarsky
Whiteboard: [need review]
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.
Attachment #668714 - Flags: approval-mozilla-beta?
Attachment #668714 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/1c64ccca6860
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [need review]
Target Milestone: --- → mozilla18
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?
Flags: in-testsuite?
Keywords: testcase
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.
Attachment #668714 - Flags: approval-mozilla-beta?
Attachment #668714 - Flags: approval-mozilla-beta-
Attachment #668714 - Flags: approval-mozilla-aurora?
Attachment #668714 - Flags: approval-mozilla-aurora+
(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.
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.
Keywords: verifyme
Virgil, please keep the verifyme keyword on this bug until it's been verified across all fixed branches.
Keywords: verifyme
(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
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.