crash in mozilla::layout::GetFrom(nsFrameLoader*)

RESOLVED FIXED in Firefox 42



Graphics: Layers
3 years ago
3 years ago


(Reporter: mats, Assigned: nical)


({crash, regression})

42 Branch
crash, regression

Firefox Tracking Flags

(firefox41 unaffected, firefox42+ fixed, firefox43+ fixed)


(Whiteboard: gfx-noted, crash signature)


(3 attachments)



3 years ago
This bug was filed from the Socorro interface and is 
report bp-e81d857c-dfcf-4746-9090-f9edc2150825.

This crash appears to be a regression in v42:

Product 	Version 	Percentage 	Number Of Crashes
Firefox 	42.0a1 	36.79 %	78
Firefox 	42.0a2 	34.91 %	74
Firefox 	43.0a1 	22.64 %	48
Firefox 	41.0a2 	5.66 %	12
(no reported incidents in older versions; numbers are for 28 days)

All crash addresses are 0x48 or 0x40, so I'm guessing 'mFrameLoader'
is null here:

nsXULElement::UnbindFromTree(bool, bool)
mozilla::dom::Element::UnbindFromTree(bool, bool)
mozilla::dom::Element::UnbindFromTree(bool, bool)
mozilla::dom::Element::UnbindFromTree(bool, bool)
mozilla::dom::Element::UnbindFromTree(bool, bool)
mozilla::dom::Element::UnbindFromTree(bool, bool)
mozilla::dom::Element::UnbindFromTree(bool, bool)
nsXBLBinding::UninstallAnonymousContent(nsIDocument*, nsIContent*)
nsXBLBinding::ChangeDocument(nsIDocument*, nsIDocument*)
nsBindingManager::RemovedFromDocumentInternal(nsIContent*, nsIDocument*)

Comment 1

3 years ago
The "GetFrom(mFrameLoader)" call was added here:
for bug 918634, so it might be a regression in v36 although it's only showing up
in statistics now when e10s is more widely enabled?
Flags: needinfo?(davidp99)
Whiteboard: gfx-noted

Comment 2

3 years ago
It appears the assignee in bug 918634 (David Parks) isn't active anymore.

Nicolas, do you know anyone who can investigate this?  I'm not really sure who
knows this code.
Flags: needinfo?(davidp99) → needinfo?(nical.bugzilla)

Comment 3

3 years ago
Created attachment 8656480 [details] [diff] [review]
null-check mFrameLoader

I don't know who owns this code (that is still active), but maybe Sotaro knows it enough that he can do the review.

Looking at the code it seems fair to just null-check and early return. For one we null-check the result of GetFrom, and the only ways I can see mFrameLoader being null (by reading the code) is if ActorDestroy has already run or if the creation of the object failed. Either way we should not be running the code that is executed in OwnerContentChanged. I don't know this code enough to know if this is the symptom of another bug, but null-checking here is simple enough that it should not cause any issue (other than potentially hiding the symptom of a bug) and will get rid of this crash.
Assignee: nobody → nical.bugzilla
Flags: needinfo?(nical.bugzilla)
Attachment #8656480 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8656480 [details] [diff] [review]
null-check mFrameLoader

Review of attachment 8656480 [details] [diff] [review]:

Looks good.
Attachment #8656480 - Flags: review?(sotaro.ikeda.g) → review+
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Comment 7

3 years ago
We still crash from the call in GetTextureFactoryIdentifier:
Flags: needinfo?(nical.bugzilla)
Resolution: FIXED → ---


3 years ago
Crash Signature: [@ mozilla::layout::GetFrom(nsFrameLoader*)] → [@ mozilla::layout::GetFrom(nsFrameLoader*)] [@ nsFrameLoader::GetOwnerDoc() ]

Comment 8

3 years ago
Created attachment 8658603 [details] [diff] [review]
Another null-check

I could have seen that one coming.
Basically, in TabChild::ProvideWindow we try to create a PRenderFrame actor pair. It fails on the host side because TabParent::GetFrameLoader returns nullptr in TabParent::AllocPRenderFrameParent. so we early-return in the constructor but the PRenderFrameParen is created still with aSuccess set to fals which means AddTabParentToTable is not called but the actor is still created. Back in TabChild::ProvideWindowCommon we immediately send a synchronous ipc to query some of RenderFrameParent's info, which leads to the crashing function GetTextureFactoryIdentifier.

If we add an extra null-check here we should be good because the following call to GetLayersId() will return 0 which will be interpreted as an error in TabChild::ProvideWindowCommon and the latter will then deallocate the PRenderFrame actor pair, and mFrameLoader won't be read again.

As to why is TabParent::GetFrameLoader, I have no idea and there are many paths in there that could lead to this so I can't get much further without reproducing locally. Hg history vaguely points to bug 1126089 (introduced some of the nullptr paths).
Flags: needinfo?(nical.bugzilla)
Attachment #8658603 - Flags: review?(sotaro.ikeda.g)


3 years ago
Attachment #8658603 - Flags: review?(sotaro.ikeda.g) → review+
[Tracking Requested - why for this release]:
status-firefox42: --- → affected
tracking-firefox42: --- → ?
tracking-firefox43: --- → +
status-firefox41: --- → unaffected
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Crash, tracking.

Nical, could you fill the uplift request to aurora? Thanks
tracking-firefox42: ? → +
Flags: needinfo?(nical.bugzilla)

Comment 13

3 years ago
Created attachment 8663586 [details] [diff] [review]
Folded patch for aurora

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: crashes
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low risk, just a few null checks
[String/UUID change made/needed]: none
Flags: needinfo?(nical.bugzilla)
Attachment #8663586 - Flags: approval-mozilla-aurora?
Attachment #8663586 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+


3 years ago
status-firefox42: affected → fixed
You need to log in before you can comment on or make changes to this bug.