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

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Kamil Jozwiak [:kjozwiak]]

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]

Comment 1

[Kamil Jozwiak [:kjozwiak]]

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.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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?

Comment 4

[Jonathan Hao (inactive) [:jhao]]

(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.

Comment 5

[Jonathan Hao (inactive) [:jhao]]

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?

Comment 6

[Andrea Marchesini [:baku]]

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.

Comment 7

[Jonathan Hao (inactive) [:jhao]]

Hi Boris, what do you think about Comment 5?

Comment 8

[Boris Zbarsky [: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.

Comment 9

[Jonathan Hao (inactive) [:jhao]]

Attachment: 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?

Comment 10

[Jonathan Hao (inactive) [:jhao]]

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 11

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 12

[Jonathan Hao (inactive) [:jhao]]

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.

Comment 13

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f590e9739d05

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 15

[Kamil Jozwiak [:kjozwiak]]

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]

Comment 16

[Ethan Tseng [:ethan]]

(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.

Comment 17

[Jonathan Hao (inactive) [:jhao]]

Attachment: 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?

Comment 18

[Jonathan Hao (inactive) [:jhao]]

(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.

Comment 19

[Andrea Marchesini [:baku]]

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.

Comment 20

[Kamil Jozwiak [:kjozwiak]]

> 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.

Comment 21

[Jonathan Hao (inactive) [:jhao]]

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.

Comment 22

[Jonathan Hao (inactive) [:jhao]]

Attachment: 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.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8753639 [details] [diff] [review]
Revert changes in bug 1260766 about nsFrameLoader::Swap

baku could review this.

Comment 24

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0cbdc5c1423

Comment 25

[Jonathan Hao (inactive) [:jhao]]

Attachment: Set correct usercontextid before swapping tabs

Update commit message

Comment 26

[Jonathan Hao (inactive) [:jhao]]

Attachment: Revert changes in bug 1260766 about nsFrameLoader::Swap

Update commit message.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cba66ab3465
https://hg.mozilla.org/integration/mozilla-inbound/rev/8532f8ec8acd

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8532f8ec8acd

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7cba66ab3465

Comment 30

[Codacoder]

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

[Codacoder]

(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")

Comment 32

[Mike Taylor [:miketaylr]]

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

[Codacoder]

Created separate bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1282589