Closed Bug 1183491 Opened 9 years ago Closed 9 years ago

crash in nsGenericHTMLFrameElement EXCEPTION_ACCESS_VIOLATION_READ

Categories

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

40 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: marvinhk, Assigned: bzbarsky)

Details

Crash Data

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150709163524

Steps to reproduce:

1. pref: don't load tab until selected
2. start firefox with session restore (6 windows with around 10 tabs each)
3. for each windows, right click on the 1st (loaded) tab and "Reload All Tabs"




Actual results:

crash occurs when the tabs are refreshing

note: if the number of windows to be restored increased, it becomes a start-up crash



Expected results:

tabs are reloaded succesfully without crash
Crash Signature: [@ nsGenericHTMLFrameElement::GetContentDocument() | mozilla::dom::HTMLIFrameElementBinding::get_contentDocument]
Component: General → DOM
Crash Signature: [@ nsGenericHTMLFrameElement::GetContentDocument() | mozilla::dom::HTMLIFrameElementBinding::get_contentDocument] → [@ nsGenericHTMLFrameElement::GetContentDocument() ]
Do you have a link from about:crashes?
Flags: needinfo?(marvinhk)
crash report is here <https://crash-stats.mozilla.com/report/index/1ba0691d-815d-4f35-b241-853d22150714>
Flags: needinfo?(marvinhk)
Thanks, and sorry for the lag on my end here....

This looks like a null-deref on the "doc->NodePrincipal()" line.  That's a bit confusing to me; I would have expected GetContentWindow to return null if we no longer have a window with a document...

In any case, I guess the right thing here is to add a null-check.

Also, I expect there's an extension involved, since the get is coming from a scripted content policy.  So it's possible that the extension will end up with exceptions (instead of this crash) if it doesn't null-check the .contentDocument it's getting.
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8640034 [details] [diff] [review]
Null-check the document we get from our contentWindow in the contentDocument getter, because apparently it can end up null

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

This looks reasonable, but I can't help but wonder if we should be setting mDoc here:

  http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#3440

That comment looks bogus.  I can't find where docshell calls SetNewDocument().
Attachment #8640034 - Flags: review?(bkelly) → review+
> I can't find where docshell calls SetNewDocument().

nsDocShell::GetDocument calls EnsureContentViewer() (carefully hidden in the NS_ENSURE_SUCCESS macro) which calls CreateAboutBlankContentViewer which ends up doing various stuff that lands in nsDocumentViewer::InitInternal which calls SetNewDocument.
just additional information: firefox lags a lot and crashes in nsGenericHTMLFrameElement starts happening since ff40. it is possible to be a side effect of omtc being enabled by default in ff40. perf off otmc at the moment to see if this continues to happen.
https://hg.mozilla.org/mozilla-central/rev/87121bfe4be9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
seems the issue is still there in ff41 <https://crash-stats.mozilla.com/report/index/bf749a0a-a809-4beb-a941-9bab62150826>
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: