Closed Bug 1009624 Opened 6 years ago Closed 6 years ago

startup crash in nsXULPrototypeDocument::GetCompilationGlobal()

Categories

(Core :: XUL, defect, critical)

29 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox29 --- wontfix
firefox30 + verified
firefox31 --- verified
firefox32 --- verified
b2g-v1.4 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

(Keywords: crash, topcrash-win)

Crash Data

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #994335 +++

There are similar but not the same stacks as what Bug 994335 was filed for.
This bug is about the following kinds of crashes.
(Unfortunately crash-stat doesn't give sane line numbers)


This bug was filed from the Socorro interface and is 
https://crash-stats.mozilla.com/report/index/5749d746-5061-4f94-bd32-f6d932140506
=============================================================


Frame 	Module 	Signature 	Source
0 	xul.dll 	nsXULPrototypeDocument::GetCompilationGlobal() 	content/xul/document/src/nsXULPrototypeDocument.cpp
1 	xul.dll 	nsXULPrototypeScript::Compile(wchar_t const *,int,nsIURI *,unsigned int,nsIDocument *,nsXULPrototypeDocument *,nsIOffThreadScriptReceiver *) 	content/xul/content/src/nsXULElement.cpp
2 	xul.dll 	mozilla::dom::XULDocument::OnStreamComplete(nsIStreamLoader *,nsISupports *,tag_nsresult,unsigned int,unsigned char const *) 	content/xul/document/src/XULDocument.cpp
3 	xul.dll 	nsStreamLoader::OnStopRequest(nsIRequest *,nsISupports *,tag_nsresult) 	netwerk/base/src/nsStreamLoader.cpp
4 	xul.dll 	nsJARChannel::OnStopRequest(nsIRequest *,nsISupports *,tag_nsresult) 	modules/libjar/nsJARChannel.cpp
5 	xul.dll 	nsInputStreamPump::OnStateStop() 	netwerk/base/src/nsInputStreamPump.cpp
6 	xul.dll 	nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream *) 	netwerk/base/src/nsInputStreamPump.cpp
7 	xul.dll 	nsInputStreamReadyEvent::Run() 	xpcom/io/nsStreamUtils.cpp
8 	xul.dll 	nsThread::ProcessNextEvent(bool,bool *) 	xpcom/threads/nsThread.cpp
9 	xul.dll 	NS_ProcessNextEvent(nsIThread *,bool) 	xpcom/glue/nsThreadUtils.cpp
10 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) 	ipc/glue/MessagePump.cpp
(In reply to Olli Pettay [:smaug] from comment #0)
> (Unfortunately crash-stat doesn't give sane line numbers)

Does looking into minidumps via a debugger help there?
No idea.
BTW, brian, does the crash ring any bells. This is a regression from Bug 897655, as far as I see.
Flags: needinfo?(bhackett1024)
No, it doesn't.  Bug 897655 made some changes around here but why would it suddenly start crashing 6 months after landing?
Flags: needinfo?(bhackett1024)
Attached patch null checkSplinter Review
Given that Bug 994335 had the same crash address 0x18 we could try this kind
of patch. Still no idea how we could in either bugs end up to
null proto.
Attachment #8421914 - Flags: review?(bobbyholley)
(In reply to Brian Hackett (:bhackett) from comment #4)
> No, it doesn't.  Bug 897655 made some changes around here but why would it
> suddenly start crashing 6 months after landing?

We have seen this bug for ages, starting from last August.
Er, we've seen this or bug 994335 since last August.
Attachment #8421914 - Flags: review?(bobbyholley) → review+
Ok, this is actually an ancient bug.
The code just used to live in a different place
http://hg.mozilla.org/releases/mozilla-release/annotate/d20d499b219f/content/xul/document/src/XULDocument.cpp#l3536 is the same issue.
Keywords: leave-open
Bug 993772 has probably fixed this.
Better patch coming, for beta. This should be fixed elsewhere.

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=19bd36676edf
(In reply to Olli Pettay [:smaug] from comment #12)
> Better patch coming
Or not. No idea yet how to come up with something safe and certainly non-leaky.


Kairo, how badly does this show up in beta? This one should be fixed on Aurora already.
Flags: needinfo?(kairo)
Attached patch v2Splinter Review
Or I guess this might be safe enough.
Just use the global of the master prototype.
The document should always have the master, but may not have current prototype.

(Trying to get this pushed to try, but it is closed)
Attachment #8422081 - Flags: review?(bobbyholley)
(In reply to Olli Pettay [:smaug] from comment #10)
> Er, crash-stat link
> https://crash-stats.mozilla.com/report/index/e61366d6-abdd-4f3b-9e67-159172140513
> to the old crash.

Interestingly, mozilla::dom::XULDocument::OnStreamComplete was a low-level signature, low enough that we never filed a bug for it, but nsXULPrototypeDocument::GetCompilationGloba is pretty high up in the stats.

(In reply to Olli Pettay [:smaug] from comment #13)
> Kairo, how badly does this show up in beta? This one should be fixed on
> Aurora already.

It's #15 in 29.0.1 release, #11 in 30 beta, but you are right in that it does not show up on 31 Aurora or 32 Nightly any more.
Crash Signature: [@ nsXULPrototypeDocument::GetCompilationGlobal()] → [@ nsXULPrototypeDocument::GetCompilationGlobal()] [@ mozilla::dom::XULDocument::OnStreamComplete(nsIStreamLoader*, nsISupports*, tag_nsresult, unsigned int, unsigned char const*)]
Flags: needinfo?(kairo)
Comment on attachment 8422081 [details] [diff] [review]
v2

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

I guess these compilations globals are all equivalent anyway. This looks fine for beta.

Can you also file a bug (and probably mark it as mentor= for removing the now-unnecessary aProtoDoc argument to ::Compile on master?
Attachment #8422081 - Flags: review?(bobbyholley) → review+
Comment on attachment 8422081 [details] [diff] [review]
v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): ancient stuff, but something has
made this happen more often recently
User impact if declined: crashes
Testing completed (on m-c, etc.): just landed to m-i
Risk to taking this patch (and alternatives if risky): This is the safest fix I can think of, but this code is old and we don't have too many tests for it. So, a bit risky.
String or IDL/UUID changes made by this patch: NA

Note, this bug has been fixed on trunk and Aurora already, but I landed the patch
to m-i too in order to see if NS_ENSURE_STATE(aProtoDoc); causes issues.
(Passing PrototypeDocument to Compile is no-op in Aurora and trunk).

We don't have any way to reproduce this atm, but based on stack traces and code inspection this should help.
Attachment #8422081 - Flags: approval-mozilla-beta?
Attachment #8422081 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/b2d759db4dcc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Olli, 31 is marked as unaffected, is there a reason why you request the uplift for aurora then? I am going to approve only beta for now.
Flags: needinfo?(bugs)
Attachment #8422081 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Because I'd like to see if NS_ENSURE_STATE(aProtoDoc); causes any problems.
Flags: needinfo?(bugs)
Attachment #8422081 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Olli Pettay [:smaug] from comment #19)
> We don't have any way to reproduce this atm, but based on stack traces and
> code inspection this should help.
marking as [qa-]
Keywords: verifyme
Whiteboard: [qa-]
We can observe crash stats to verify this is fixed. It is on my radar for the next beta(6) as this crash is currently #11 on Fx30.
QA Whiteboard: [qa+]
Whiteboard: [qa-]
crash stats show that this has been fixed across all channels, including no crashes with Fx30b6.
QA Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.