Closed
Bug 1266484
Opened 8 years ago
Closed 8 years ago
crash in libsystem_kernel.dylib@0x16db6 when using Pinboard bookmarklet
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
e10s | ? | --- |
firefox45 | --- | unaffected |
firefox46 | --- | unaffected |
firefox47 | --- | unaffected |
firefox48 | + | fixed |
firefox49 | --- | fixed |
People
(Reporter: cpeterson, Assigned: haik)
References
(Blocks 1 open bug)
Details
(Keywords: crash, regression, reproducible, Whiteboard: btpp-fixnow)
Crash Data
Attachments
(1 file, 2 obsolete files)
This bug was filed from the Socorro interface and is report bp-d9a3ec18-9358-4abb-af89-2e67e2160421. ============================================================= STR: 1. You need a pinboard.in account. 2. Copy the Pinboard "read later" bookmarklet to your bookmark toolbar: https://pinboard.in/howto/#saving 3. Click the "read later" bookmarklet. RESULT: The bookmarklet will pop up a small window, as expected, and then the content process will crash.
Reporter | ||
Comment 1•8 years ago
|
||
[Tracking Requested - why for this release]: Haik, I suspect this crash is a regression from fix for window resizing bug 1258925. I bisected this crash to the following pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a338aa9ac43ff12190109267d2210d230db9286e&tochange=a00a1a9a5a7f947c987b7e8c79182ceefa0f7f18
Blocks: 1258925
tracking-firefox48:
--- → ?
Flags: needinfo?(haftandilian)
Keywords: regressionwindow-wanted
Reporter | ||
Comment 2•8 years ago
|
||
btw, I also noticed some strange page layout issues when switching tabs this morning. The page elements slowly moved from the side of the page to their correct positions, as if they were adapting to an expanding window size, even though the window size was already maximized. I don't know if that layout issue is related to this crash.
Component: IPC → DOM
Reporter | ||
Comment 3•8 years ago
|
||
This crash only happens when e10s is enabled.
tracking-e10s:
--- → ?
Keywords: reproducible
Updated•8 years ago
|
Whiteboard: btpp-fixnow
Assignee | ||
Comment 4•8 years ago
|
||
The crashing thread was in PBrowserChild::SendGetTabCount(unsigned int*) which was added by my fix for 1258925. I'll take this one.
Flags: needinfo?(haftandilian)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → haftandilian
Assignee | ||
Comment 5•8 years ago
|
||
On the chrome side, when handling the GetTabCount message, we return an error from RecvGetTabCount which triggers the content process being crashed. RecvGetTabCount returns an error because TabParent::GetXULBrowserWindow returns NULL due to the docShell having a NULL mTreeOwner. I don't understand why we don't have an mTreeOwner yet.
Updated•8 years ago
|
Crash Signature: [@ libsystem_kernel.dylib@0x16db6] → [@ libsystem_kernel.dylib@0x16db6]
[@ WaitForSingleObjectEx | WaitForSingleObject | PR_WaitCondVar | mozilla::CondVar::Wait | mozilla::ipc::MessageChannel::WaitForSyncNotify | mozilla::ipc::MessageChannel::Send | mozilla::dom::PBrowserChild::SendGetTabCo…
Assignee | ||
Comment 6•8 years ago
|
||
Something about this test case causes the docShell's mTreeOwner field to be NULL and without the fix for 1258925, we have the same issue but it doesn't cause a crash because TabParent::RecvSetDimensions() ignores the resize and returns true. More investigation is needed to determine if that's a bug. For example, are resizes of some popups being ignored? I think it makes sense to fix the crash by taking the same approach in TabParent::RecvGetTabCount() and returning true in this case. Then looking into why the mTreeOwner member is NULL and determining if that's a existing bug.
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8744200 -
Flags: review?(gkrizsanits)
Comment 8•8 years ago
|
||
Comment on attachment 8744200 [details] [diff] [review] Changes RecvGetTabCount to return true when the docShell has no tree owner Review of attachment 8744200 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabParent.cpp @@ -3251,4 @@ > > uint32_t tabCount; > nsresult rv = xulBrowserWindow->GetTabCount(&tabCount); > NS_ENSURE_SUCCESS(rv, false); Could you change this one too?
Attachment #8744200 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8) > Comment on attachment 8744200 [details] [diff] [review] > Changes RecvGetTabCount to return true when the docShell has no tree owner > > Review of attachment 8744200 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/ipc/TabParent.cpp > @@ -3251,4 @@ > > > > uint32_t tabCount; > > nsresult rv = xulBrowserWindow->GetTabCount(&tabCount); > > NS_ENSURE_SUCCESS(rv, false); > > Could you change this one too? Sure, but I don't have a good idea of what would cause that GetTabCount call to fail and if it's something we would want to trigger a content crash for. Ignoring it seems harmless though. I think it's more important to figure out why the docShell's mTreeOwner is NULL. I'll update the patch to return true for GetTabCount failures and to also set the tabCount to zero when the function starts and add a comment explaining a zero tabCount means we failed to get the number of tabs for the window.
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8744200 -
Attachment is obsolete: true
Attachment #8744465 -
Flags: review?(gkrizsanits)
Comment 11•8 years ago
|
||
Comment on attachment 8744465 [details] [diff] [review] Changes RecvGetTabCount to return true/tabCount=0 when GetXULBrowserWindow or GetTabCount fails Review of attachment 8744465 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Haik Aftandilian [:haik] from comment #9) > Sure, but I don't have a good idea of what would cause that GetTabCount call > to fail and if it's something we would want to trigger a content crash for. I just want to avoid confusion with it. The other option is to not check the return value at all. The advantage of this version is that it's more future proof. > Ignoring it seems harmless though. I think it's more important to figure out > why the docShell's mTreeOwner is NULL. Most of the recv functions are doing the same thing so I wouldn't worry too much about it. If we return null we let the change happen, if there is no other reason to block (we would only block the request if tab count > 1). What important is that if there are multiple tabs in the docshell, then we return something bigger than 1. > > I'll update the patch to return true for GetTabCount failures and to also > set the tabCount to zero when the function starts and add a comment > explaining a zero tabCount means we failed to get the number of tabs for the > window. This sounds good, thanks. ::: dom/ipc/TabParent.cpp @@ +3246,5 @@ > +/** > + * Returns the number of tabs in the window via the out parameter. > + * If the number of tabs can't be determined, returns 0. > + * > + * @param aValue where to store the tab count Method documentations like this usually goes to the header.
Attachment #8744465 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 12•8 years ago
|
||
Moved comment to .ipdl file. https://treeherder.mozilla.org/#/jobs?repo=try&revision=36e9dc4985ae
Attachment #8744465 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/df963d1bc915
Keywords: checkin-needed
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/df963d1bc915
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Target Milestone: mozilla48 → mozilla49
Haik, this was mentioned as a top crasher on Aurora48. Do you think the fix is safe and the Nightly data looking promising so we can uplift this to Aurora?
Flags: needinfo?(haftandilian)
Assignee | ||
Comment 16•8 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #15) > Haik, this was mentioned as a top crasher on Aurora48. Do you think the fix > is safe and the Nightly data looking promising so we can uplift this to > Aurora? Yes, I think it's OK to uplift this to 48. The fix is low risk.
Flags: needinfo?(haftandilian)
Updated•8 years ago
|
Crash Signature: mozilla::dom::PBrowserChild::SendGetTabCount]
[@ WaitForSingleObjectEx | PR_WaitCondVar | mozilla::CondVar::Wait | mozilla::ipc::MessageChannel::WaitForSyncNotify | mozilla::ipc::MessageChannel::Send | mozilla::dom::PBrowserChild::SendGetTabCount] → mozilla::dom::PBrowserChild::SendGetTabCount]
[@ WaitForSingleObjectEx | PR_WaitCondVar | mozilla::CondVar::Wait | mozilla::ipc::MessageChannel::WaitForSyncNotify | mozilla::ipc::MessageChannel::Send | mozilla::dom::PBrowserChild::SendGetTabCount]
[@ I…
For it to be uplifted, you should request approval for aurora.
Flags: needinfo?(haftandilian)
Assignee | ||
Updated•8 years ago
|
Attachment #8744954 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(haftandilian)
Assignee | ||
Updated•8 years ago
|
Attachment #8744954 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 18•8 years ago
|
||
Removed approval-mozilla-aurora? while I sanity test on Aurora.
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8744954 [details] [diff] [review] Changes RecvGetTabCount to return true/tabCount=0 when GetXULBrowserWindow or GetTabCount fails Approval Request Comment [Feature/regressing bug #]:1258925 [User impact if declined]:Content process crashes on some pages that trigger popups hence bad user experience. [Describe test coverage new/current, TreeHerder]:In mozilla-central since 2016-04-26. Nightly try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36e9dc4985ae Inbound try: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=df963d1bc915 An earlier version of the fix, but results still relevant: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e154f1c3c0e [Risks and why]:Low risk. The changes are small and only affect the area of code added for 1258925. [String/UUID change made/needed]:N/A
Attachment #8744954 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 21•8 years ago
|
||
Patch applies cleanly to Aurora. Did manual testing confirming the problem exists in Aurora, but is not reproducible in debug or non-debug with the patch applied.
Comment 22•8 years ago
|
||
Comment on attachment 8744954 [details] [diff] [review] Changes RecvGetTabCount to return true/tabCount=0 when GetXULBrowserWindow or GetTabCount fails Crash fix, manual testing on aurora, let's uplift it!
Attachment #8744954 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 24•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/36e12a7d0ae8
Updated•8 years ago
|
Version: unspecified → Trunk
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•