Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Regression: mozilla::dom::TabChild::RecvSwappedWithOtherRemoteLoader crash when moving container tabs

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Security
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kjozwiak, Assigned: jhao)

Tracking

(Blocks: 1 bug)

49 Branch
mozilla49
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [userContextId][domsecurity-active])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

a year ago
It looks like container tabs are currently crashing when they're dragged from the main window into their own separate window.

STR:

* launch m-c
* open a new container tab in the main window and load any website within that container
* once the website has loaded, drag the container tab into it's own separate window
* you should receive an instant crash within the container that was moved

Crashes on the latest version of m-c:

* https://crash-stats.mozilla.com/report/index/b1e04bf5-6c40-4e96-8baf-6eb862160510
* https://crash-stats.mozilla.com/report/index/52f5f1a0-eba7-411a-8069-b5eed2160510

Stack produced by a m-c debug build:

Hit MOZ_CRASH(Update to TabContext after swap was denied.) at /Users/kjozwiak/projects/m-c-containers/dom/ipc/TabChild.cpp:2322
#01: mozilla::dom::TabChild::RecvSwappedWithOtherRemoteLoader(mozilla::dom::IPCTabContext const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3f3019f]
#02: mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1079c1d]
#03: mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1155592]
#04: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xb235c3]
#05: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xb227dc]
#06: mozilla::ipc::MessageChannel::OnMaybeDequeueOne()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xb1d2c2]
#07: decltype(*(fp).*fp0(mozilla::Get<>(fp1).PassAsParameter())) nsRunnableMethodArguments<>::applyImpl<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()>(mozilla::ipc::MessageChannel*, bool (mozilla::ipc::MessageChannel::*)(), mozilla::Tu[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xb455ba]
#08: _ZN25nsRunnableMethodArgumentsIJEE5applyIN7mozilla3ipc14MessageChannelEMS4_FbvEEEDTcl9applyImplfp_fp0_dtdefpT10mArgumentscvNS2_13IndexSequenceIJEEE_EEEPT_T0_[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xb45529]
#09: nsRunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xb452f2]
#10: mozilla::ipc::MessageChannel::RefCountedTask::Run()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xb405e2]
#11: mozilla::ipc::MessageChannel::DequeueTask::Run()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xb40431]
#12: MessageLoop::RunTask(already_AddRefed<mozilla::Runnable>)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xa70902]
#13: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask&&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xa70e29]
#14: MessageLoop::DoWork()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xa71016]
#15: mozilla::ipc::DoWorkRunnable::Run()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xb267ae]
#16: nsThread::ProcessNextEvent(bool, bool*)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1a68c1]
#17: NS_ProcessPendingEvents(nsIThread*, unsigned int)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x22c63c]
#18: nsBaseAppShell::NativeEventCallback()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x43510ce]
#19: nsAppShell::ProcessGeckoEvents(void*)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x43e8c12]
#20: __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0xaa881]
#21: __CFRunLoopDoSources0[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x89fbc]
#22: __CFRunLoopRun[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x894df]
#23: CFRunLoopRunSpecific[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x88ed8]
#24: RunCurrentEventLoopInMode[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x30935]
#25: ReceiveNextEventCommon[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x3076f]
#26: _BlockUntilNextEventMatchingListInModeWithFilter[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x305af]
#27: _DPSNextEvent[/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x48efa]
#28: -[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:][/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x4832a]
#29: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:][/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x43e7704]
#30: -[NSApplication run][/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x3ce84]
#31: nsAppShell::Run()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x43e95dc]
#32: XRE_RunAppShell[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x55fbcb5]
#33: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xb26a0f]
#34: MessageLoop::RunInternal()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xa707a5]
#35: MessageLoop::RunHandler()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xa70705]
#36: MessageLoop::Run()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0xa706ad]
#37: XRE_InitChildProcess[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x55fb511]
#38: content_process_main(int, char**)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/plugin-container.app/Contents/MacOS/plugin-container +0x85ad]
#39: main[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/plugin-container.app/Contents/MacOS/plugin-container +0x8692]
(Reporter)

Comment 1

a year ago
Received the following regression range while using mozregression:

24:43.03 INFO: Last good revision: a9f6e41e3250980ad72367d2e4f123f3c7bc0cbe
24:43.03 INFO: First bad revision: 88d9ca956c2ca82816ffa85cb286361e9ec4c9cf
24:43.03 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a9f6e41e3250980ad72367d2e4f123f3c7bc0cbe&tochange=88d9ca956c2ca82816ffa85cb286361e9ec4c9cf

Looks like bug#1268688 is causing the tab crash.
Whiteboard: [usercontextId]
Before bug 1268688, the TabContext in the child was never updated at all with swapFrameLoaders.  This was tweaked to allow only the mIsMozBrowserElement property to be updated, but it also checks to see if all other properties remained the same[1].

So, I assume for this case, the OriginAttrs (with the user context) inside did not match?

[1]: https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabContext.cpp#168-175
I see that bug 1260766 adjusted the OriginAttr checks in swapFrameLoaders to also read the usercontextid attribute before deciding if the swap is okay.

But this just computes an adjusted OriginAttr for the purposes of seeing if it is safe to proceed with the swap.  Is there also a code path that updates the docshell in the child to actually apply the new user context?
Flags: needinfo?(amarchesini)
Priority: -- → P1
Summary: mozilla::dom::TabChild::RecvSwappedWithOtherRemoteLoader crash when moving container tabs → Regression: mozilla::dom::TabChild::RecvSwappedWithOtherRemoteLoader crash when moving container tabs
(Assignee)

Updated

a year ago
Assignee: nobody → jhao
(Reporter)

Updated

a year ago
QA Contact: kjozwiak
Whiteboard: [usercontextId] → [userContextId][domsecurity-active]
(Assignee)

Comment 4

a year ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> Before bug 1268688, the TabContext in the child was never updated at all
> with swapFrameLoaders.  This was tweaked to allow only the
> mIsMozBrowserElement property to be updated, but it also checks to see if
> all other properties remained the same[1].
> 
> So, I assume for this case, the OriginAttrs (with the user context) inside
> did not match?
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabContext.cpp#168-175

I reproduced this crash in rr. When the crash happened, the TabChild's userContext was 0 (default). It confirmed that we did not apply the new user context to the child. However, I'm not sure what the correct code path should be.
(Assignee)

Comment 5

a year ago
If there isn't a code path to update the child, can we change the behavior of TabChild::RecvSwappedWithOtherRemoteLoader() such that when we get a mismatching userContextId, instead of crashing, we set the child's userContextId to what we received. Is this reasonable?
I don't know if this is the correct approach, but we can set the OriginAttributes in the docShell: nsDocShell::SetOriginAttributes. You should ask a docShell peer if this is correct, but in case, just set the new OriginAttributes contained in the TabContext dictionary.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 7

a year ago
Hi Boris, what do you think about Comment 5?
Flags: needinfo?(bzbarsky)
I don't know enough about what the relevant bits are and how they're supposed to interact to have an opinion here.  Assuming you actually meant comment 5.  If you meant comment 6, more or less the same answer, because I'm not sure what the tabcontext really stores.

But conceptually I believe the right thing to do is to ensure that we only swap between things that have identical origin attributes.  That means that before you go to swap you should be setting things up in the right state, however you get there.
Flags: needinfo?(bzbarsky)
Blocks: 1250063
No longer blocks: 1250063
(Assignee)

Comment 9

a year ago
Created attachment 8752152 [details] [diff] [review]
Set correct usercontextid before swapping tabs

This patch should set things up in the right state before wapping.

Olli, would you take a look at this patch, please?
Attachment #8752152 - Flags: review?(bugs)
(Assignee)

Comment 10

a year ago
The previous patch avoids the crash but then, sometimes an assertion failure happens:
https://hg.mozilla.org/mozilla-central/rev/e7bfb11e947d

Although this assertion was backed out later, it's still something to worry. I'm not sure what I did wrong, though.
Comment on attachment 8752152 [details] [diff] [review]
Set correct usercontextid before swapping tabs

baku has been looking at usercontextid stuff recently. Perhaps he could review this. Or some browser peer.
Attachment #8752152 - Flags: review?(bugs) → review?(amarchesini)
Attachment #8752152 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 12

a year ago
Thanks to smaug and baku.

I'd like to write an automation test for this, though I don't know if it's possible. I can only test it manually right now.

Kamil, would you please double-check the patch to test if it's actually working? Thank you.
Flags: needinfo?(kjozwiak)
(Assignee)

Comment 13

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f590e9739d05
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
With this change in place, is it possible to additionally revert the changes[1] from bug 1260766?

It seems like you are now setting the user context early enough that it is part of the OriginAttrs from the beginning, so the special case added in bug 1260766 may not be needed anymore.  Reverting bug 1260766 should simplify the swapFrameLoaders code path, assuming it's possible to do.

[1]: https://hg.mozilla.org/mozilla-central/rev/e78a24e7938b
Flags: needinfo?(jhao)
(Reporter)

Comment 15

a year ago
Jonathan, I applied the patch from comment # 9 to the latest version of m-c and went through the test cases that are listed below. However, when moving around container tabs in a non-e10s window, I end up getting a crash which I've added the stack below. I've reproduced this several times while using a non-e10s window. However, I couldn't reproduce the issue with a e10s window and can only reproduce it reliably using Facebook. I'm not 100% sure if this is related to dragging containers across different windows or something related to rendering Facebook under a non-e10s window.

Test Cases Used:
================

- ensured that dragging container tabs into different/seperate windows worked correctly in both e10s/non-e10s
- ensured that opening/closing containers worked correctly
- ensured that containers kept their contextid when dragging them from one window to another window
- ensuring that opening all four containers in a single window and dragged them all out into their own seperate windows worked without issues
- ensured that you can't drag non-private container tabs into a private browsing window
- ensured that you can't drag e10s container tabs into a non-e10s window
- ensured that you can't drag non-e10s container tabs into a e10s window

Crash Info:
===========

STR:

* open the latest m-c with the patch from comment #9 applied
* open a container in a new e10s window and login into Facebook
* once logged in, close the e10s window with the container tab and open a new non-e10s window
* in the non-e10s window, open the same container that you've used before and load Facebook
* once Facebook as loaded, move the container tab into it's own window via dragging/dropping

Assertion failure: !mIsDormant (should be out of dormant by now), at /Users/kjozwiak/projects/m-c-containers/dom/media/MediaDecoder.cpp:824
#01: mozilla::MediaDecoder::Seek(double, mozilla::SeekTarget::Type)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x36cd4d4]
#02: mozilla::dom::HTMLMediaElement::Seek(double, mozilla::SeekTarget::Type, mozilla::ErrorResult&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x35319af]
#03: mozilla::dom::HTMLMediaElement::SetCurrentTime(double, mozilla::ErrorResult&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3531a8e]
#04: mozilla::dom::HTMLMediaElement::SetCurrentTime(double)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3531e78]
#05: mozilla::dom::HTMLMediaElement::MetadataLoaded(mozilla::MediaInfo const*, nsAutoPtr<nsDataHashtable<nsCStringHashKey, nsCString> const>)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3538761]
#06: mozilla::MediaDecoder::MetadataLoaded(nsAutoPtr<mozilla::MediaInfo>, nsAutoPtr<nsDataHashtable<nsCStringHashKey, nsCString> >, mozilla::MediaDecoderEventVisibility)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x36ccfed]
#07: mozilla::EnableIf<TakeArgs<void (mozilla::MediaDecoder::*)(nsAutoPtr<mozilla::MediaInfo>, nsAutoPtr<nsDataHashtable<nsCStringHashKey, nsCString> >, mozilla::MediaDecoderEventVisibility)>::value, mozilla::MediaEventListener>::Type mozilla::MediaEventSource[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3736635]
#08: void mozilla::detail::ListenerHelper<mozilla::AbstractThread, mozilla::EnableIf<TakeArgs<void (mozilla::MediaDecoder::*)(nsAutoPtr<mozilla::MediaInfo>, nsAutoPtr<nsDataHashtable<nsCStringHashKey, nsCString> >, mozilla::MediaDecoderEventVisibility)>::value[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x373655b]
#09: mozilla::detail::ListenerHelper<mozilla::AbstractThread, mozilla::EnableIf<TakeArgs<void (mozilla::MediaDecoder::*)(nsAutoPtr<mozilla::MediaInfo>, nsAutoPtr<nsDataHashtable<nsCStringHashKey, nsCString> >, mozilla::MediaDecoderEventVisibility)>::value, moz[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3736327]
#10: mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1b57f5]
#11: nsThread::ProcessNextEvent(bool, bool*)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1a75a1]
#12: NS_ProcessPendingEvents(nsIThread*, unsigned int)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x22db6c]
#13: nsBaseAppShell::NativeEventCallback()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4369d7e]
#14: nsAppShell::ProcessGeckoEvents(void*)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4401b62]
#15: __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0xaa881]
#16: __CFRunLoopDoSources0[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x89fbc]
#17: __CFRunLoopRun[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x894df]
#18: CFRunLoopRunSpecific[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x88ed8]
#19: RunCurrentEventLoopInMode[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x30935]
#20: ReceiveNextEventCommon[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x3076f]
#21: _BlockUntilNextEventMatchingListInModeWithFilter[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x305af]
#22: _DPSNextEvent[/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x48efa]
#23: -[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:][/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x4832a]
#24: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:][/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4400654]
#25: -[NSApplication run][/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x3ce84]
#26: nsAppShell::Run()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x440252c]
#27: nsAppStartup::Run()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x55583ab]
#28: XREMain::XRE_mainRun()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x560eae6]
#29: XREMain::XRE_main(int, char**, nsXREAppData const*)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x560f97c]
#30: XRE_main[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x560fe57]
#31: do_main(int, char**, char**, nsIFile*)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/firefox +0x2acd]
#32: main[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/firefox +0x1e55]
Flags: needinfo?(kjozwiak)
(In reply to Kamil Jozwiak [:kjozwiak] from comment #15)
> Jonathan, I applied the patch from comment # 9 to the latest version of m-c
> and went through the test cases that are listed below. However, when moving
> around container tabs in a non-e10s window, I end up getting a crash which
> I've added the stack below.

Hi Kamil, thanks for your testing.

I have one question. Can you reproduce this crash without Jonathan's patch?
Just want to make sure if the crash here is related to the patch.
Since the stack seems to be irrelevant to the code change in this patch.
Flags: needinfo?(kjozwiak)
(Assignee)

Comment 17

a year ago
Created attachment 8753248 [details] [diff] [review]
Set usercontextid in xul before swapping tabs (bug 1260766 reverted)

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> With this change in place, is it possible to additionally revert the
> changes[1] from bug 1260766?
> 
> It seems like you are now setting the user context early enough that it is
> part of the OriginAttrs from the beginning, so the special case added in bug
> 1260766 may not be needed anymore.  Reverting bug 1260766 should simplify
> the swapFrameLoaders code path, assuming it's possible to do.
> 
> [1]: https://hg.mozilla.org/mozilla-central/rev/e78a24e7938b

I think you're right, Ryan.

Baku, do you agree that the changes in bug 1260766 should be removed?
Attachment #8753248 - Flags: review?(amarchesini)
(Assignee)

Comment 18

a year ago
(In reply to Ethan Tseng [:ethan] from comment #16)
> (In reply to Kamil Jozwiak [:kjozwiak] from comment #15)
> > Jonathan, I applied the patch from comment # 9 to the latest version of m-c
> > and went through the test cases that are listed below. However, when moving
> > around container tabs in a non-e10s window, I end up getting a crash which
> > I've added the stack below.
> 
> Hi Kamil, thanks for your testing.
> 
> I have one question. Can you reproduce this crash without Jonathan's patch?
> Just want to make sure if the crash here is related to the patch.
> Since the stack seems to be irrelevant to the code change in this patch.

I suggest opening another bug for this crash, and maybe needinfo guys in media team. They may have a better idea about what's wrong.
Flags: needinfo?(jhao)
Comment on attachment 8753248 [details] [diff] [review]
Set usercontextid in xul before swapping tabs (bug 1260766 reverted)

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

This looks good to me, but I want bz or another docShell peer to take a look.
Attachment #8753248 - Flags: review?(amarchesini) → feedback+
(Reporter)

Comment 20

a year ago
> I have one question. Can you reproduce this crash without Jonathan's patch?
> Just want to make sure if the crash here is related to the patch.
> Since the stack seems to be irrelevant to the code change in this patch.

I went through this once again today and managed to only reproduce the media crash once after about 2-3 hours of scrolling through Facebook while I had the patch from comment #9 applied. Unfortunately, I couldn't reproduce the crash without the patch, but I did notice a similar crash via a stack that pointed to some type of media error as well. I think you're correct though, the crash usually happens after scrolling through a large amount of information on the Facebook timeline so it's probably not related to this work. We'll see what the media folks say in bug #1273612.

> I suggest opening another bug for this crash, and maybe needinfo guys in
> media team. They may have a better idea about what's wrong.

Agreed, created bug #1273612.
Flags: needinfo?(kjozwiak)
(Assignee)

Comment 21

a year ago
Comment on attachment 8753248 [details] [diff] [review]
Set usercontextid in xul before swapping tabs (bug 1260766 reverted)

(In reply to Kamil Jozwiak [:kjozwiak] from comment #20)
> Agreed, created bug #1273612.

Thank you very much, Kamil. I also talked with JW yesterday, he said he can take a look.
Attachment #8753248 - Flags: review?(bugs)
(Assignee)

Updated

a year ago
See Also: → bug 1273612
(Assignee)

Comment 22

a year ago
Created attachment 8753639 [details] [diff] [review]
Revert changes in bug 1260766 about nsFrameLoader::Swap

To make review easier, I separated the new changes and the reverting ones.

Hi Olli, since you reviewed bug 1260766, would you please review this patch that backout the changes from that bug? Thanks.
Attachment #8753639 - Flags: review?(bugs)
(Assignee)

Updated

a year ago
Attachment #8753248 - Attachment is obsolete: true
Attachment #8753248 - Flags: review?(bugs)
Comment on attachment 8753639 [details] [diff] [review]
Revert changes in bug 1260766 about nsFrameLoader::Swap

baku could review this.
Attachment #8753639 - Flags: review?(bugs) → review?(amarchesini)
Attachment #8753639 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 24

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0cbdc5c1423
(Assignee)

Comment 25

a year ago
Created attachment 8755290 [details] [diff] [review]
Set correct usercontextid before swapping tabs

Update commit message
(Assignee)

Updated

a year ago
Attachment #8752152 - Attachment is obsolete: true
(Assignee)

Comment 26

a year ago
Created attachment 8755291 [details] [diff] [review]
Revert changes in bug 1260766 about nsFrameLoader::Swap

Update commit message.
(Assignee)

Updated

a year ago
Attachment #8753639 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Attachment #8755290 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8755291 - Flags: review+

Comment 27

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cba66ab3465
https://hg.mozilla.org/integration/mozilla-inbound/rev/8532f8ec8acd
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8532f8ec8acd
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
https://hg.mozilla.org/mozilla-central/rev/7cba66ab3465
Blocks: 1275485

Comment 30

a year ago
Latest Nightly: https://crash-stats.mozilla.com/report/index/1817aefa-5747-4d17-8f8d-914812160627

STR: same as originally reported.

In support of another bug, I tested a series of try builds - one of those had this same bug. See https://bugzilla.mozilla.org/show_bug.cgi?id=1280091#c25

I'll file a separate bug if I see no traction on this bug.

Comment 31

a year ago
(In reply to Codacoder from comment #30)
> Latest Nightly:
> https://crash-stats.mozilla.com/report/index/1817aefa-5747-4d17-8f8d-
> 914812160627
> 
> STR: same as originally reported.
> 
> In support of another bug, I tested a series of try builds - one of those
> had this same bug. See
> https://bugzilla.mozilla.org/show_bug.cgi?id=1280091#c25
> 
> I'll file a separate bug if I see no traction on this bug.

I should add the following:

1 - This is 100% repeatable (here)

2 - The bug does not occur if e10s is DISABLED (no check mark in "Enable multi-precess Nightly")
Using the STR from Comment #0, I can't repro the crash in the latest Nightly (w/ e10s enabled).

Codacoder, can you file a new bug?

Comment 33

a year ago
Created separate bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1282589
You need to log in before you can comment on or make changes to this bug.