1365643 - about:newtab pages should run in content processes for activity-stream but not tiles

Status: RESOLVED FIXED

(Firefox Graveyard :: Activity Streams: General, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Activity Stream is currently preffed-off, soon to be (gradually) preffed on.  AS is e10s-capable, the old code (Tiles) is not.  So until Tiles is phased out, about:newtab should return URI_MUST_LOAD_IN_CHILD when AS is preffed on, and should not return it when AS is preffed off.  Patch forthcoming.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Bug 1365643 - Temporarily back out Activity-Stream content-process move (c17e14e79d1a),

Review commit: https://reviewboard.mozilla.org/r/140282/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140282/

Comment 2

[Ed Lee :Mardak]

Comment on attachment 8868674 [details]
Bug 1365643 - Temporarily back out Activity-Stream content-process move (c17e14e79d1a),

https://reviewboard.mozilla.org/r/140282/#review144626

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c17e14e79d1a
make activity stream, but not tiles, load in the child, r=Mardak

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failures in test_getURIFlags.js:

https://hg.mozilla.org/integration/autoland/rev/563a164d255a85725b56a9cf8d453bec07370501

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c17e14e79d1adedbbd8297126b3af0076417ea69&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=100509338&repo=autoland

[task 2017-05-19T18:03:58.788284Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/about/test/unit/test_getURIFlags.js:22:3 | 'ok' is not defined. (no-undef)
[task 2017-05-19T18:03:58.788366Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/about/test/unit/test_getURIFlags.js:32:3 | 'ok' is not defined. (no-undef)

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8868674 [details]
Bug 1365643 - Temporarily back out Activity-Stream content-process move (c17e14e79d1a),

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140282/diff/1-2/

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

A .eslint.js file was missing.  I've added on, verified that it works, and repushed via autoland.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

(Sorry about that!)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86b4df3b51cb
make activity stream, but not tiles, load in the child, r=Mardak

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/86b4df3b51cb

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Re-opening.  After discussion with :mconley, :gabor, :selena, and others, we're going to temporarily back this out until a solution for bug 1372664 is ready.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Bug 1365643 - temporarily disable preload patch until the rest of this bug relands,

Review commit: https://reviewboard.mozilla.org/r/151856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151856/

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8868674 [details]
Bug 1365643 - Temporarily back out Activity-Stream content-process move (c17e14e79d1a),

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140282/diff/2-3/

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8880523 [details]
Bug 1365643 - temporarily disable preload patch until the rest of this bug relands,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151856/diff/1-2/

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8880523 [details]
Bug 1365643 - temporarily disable preload patch until the rest of this bug relands,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151856/diff/2-3/

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

All the try tests that I ran passed; this is ready to land once it gets r+

Comment 16

[Ed Lee :Mardak]

Comment on attachment 8880523 [details]
Bug 1365643 - temporarily disable preload patch until the rest of this bug relands,

https://reviewboard.mozilla.org/r/151856/#review157228

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9377a56fe004
Temporarily back out Activity-Stream content-process move (c17e14e79d1a), r=Mardak
https://hg.mozilla.org/integration/autoland/rev/c5e8b08a4635
temporarily disable preload patch until the rest of this bug relands, r=Mardak

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Had to back out this backout for frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=109655630&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/a6e97576ead7

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

I'm a little confused about why this was backed out: looking at those failures on Treeherder, they are auto-matched to a known existing Treeherder failure: <https://bugzilla.mozilla.org/show_bug.cgi?id=1326337>.  :KWierso, can you shed some light on this?

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

OK, looking at the stack more closely, I see that although it's triggered from the same test, the crashes that triggered this backout are a bit different than the one in 1326337.

However, the top of the C++ part of the stack looks very very similar to bug 1372597, both have mozilla::widget::JumpListBuilder::DoCommitListBuild as the first non-system code on the stack of the crashing thread.

My original backout (that has now itself been backed out) had two pieces: the one that effected the code itself is conditionalized on activity-stream being turned on by default in mozilla-central, which it is not.  It seems extremely unlikely that the code changes made by my original backout somehow caused the problem.  However, my original backout also disables a test, and so it seems more plausible that disabling the test changes the test suite characteristics in a way that makes some underlying race condition (likely the same one documented in 1372597) easier to hit on Windows.  

It seems somewhat (not sure how much) likely that the race condition was introduced during the time the original patch was landed (5/22) and before I attempted the backout (6/23).

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

There were recent changes (on June 12th, in the time frame I speculated about above) in DoCommitListBuild from bug 1354143. :wcp, could the changes from that bug be causing the crashes here and in bug 1372597?

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

:jimm, :wcp, I'm speculating that this crash is likely caused by the landing in bug 1354143.  It's currently preventing us from doing the last things necessary to pref activity stream on in Nightly.  Does looking at this stack give you any thoughts on whether my hypothesis is right?
 
INFO - Crash dump filename: c:\users\cltbld\appdata\local\temp\tmpz4wbv7.mozrunner\minidumps\95626bc0-44e0-411d-9c22-c8d496c02f90.dmp
13:21:41     INFO - Operating system: Windows NT
13:21:41     INFO -                   6.1.7601 Service Pack 1
13:21:41     INFO - CPU: x86
13:21:41     INFO -      GenuineIntel family 6 model 62 stepping 4
13:21:41     INFO -      8 CPUs
13:21:41     INFO - 
13:21:41     INFO - GPU: UNKNOWN
13:21:41     INFO - 
13:21:41     INFO - Crash reason:  EXCEPTION_ACCESS_VIOLATION_READ
13:21:41     INFO - Crash address: 0x0
13:21:41     INFO - Process uptime: 151 seconds
13:21:41     INFO - 
13:21:41     INFO - Thread 68 (crashed)
13:21:41     INFO -  0  shell32.dll!SetLinkFlags + 0x15
13:21:41     INFO -     eip = 0x76e1bfcf   esp = 0x12fff2ac   ebp = 0x12fff2b8   ebx = 0x00000000
13:21:41     INFO -     esi = 0x00200000   edi = 0x00371de8   eax = 0x00000000   ecx = 0x00000000
13:21:41     INFO -     edx = 0x3a32395f   efl = 0x00010206
13:21:41     INFO -     Found by: given as instruction pointer in context
13:21:41     INFO -  1  shell32.dll!CDestinationCategory::_WriteDests(IStream *) + 0x5f
13:21:41     INFO -     eip = 0x76dfac70   esp = 0x12fff2c0   ebp = 0x12fff2e8   ebx = 0x46000000
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO -  2  shell32.dll!CDestinationCategory::_WriteCustomCategory(IStream *) + 0x22
13:21:41     INFO -     eip = 0x76de846f   esp = 0x12fff2f0   ebp = 0x12fff2f8   ebx = 0x2943a960
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO -  3  shell32.dll!CEnumFolderItemVerbs::Reset() + 0x17
13:21:41     INFO -     eip = 0x76de8443   esp = 0x12fff300   ebp = 0x12fff310
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO -  4  shell32.dll!CDestinationList::_WriteListToFile(unsigned short const *) + 0xb3
13:21:41     INFO -     eip = 0x76dfab54   esp = 0x12fff318   ebp = 0x12fff33c
13:21:41     INFO -     Found by: previous frame's frame pointer
13:21:41     INFO -  5  shell32.dll!CDestinationList::_WriteList() + 0x3e
13:21:41     INFO -     eip = 0x76dfaa4d   esp = 0x12fff344   ebp = 0x12fff55c   ebx = 0x003336e0
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO -  6  shell32.dll!CDestinationList::CommitList() + 0x77
13:21:41     INFO -     eip = 0x76dfa995   esp = 0x12fff564   ebp = 0x12fff574
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO -  7  xul.dll!mozilla::widget::JumpListBuilder::DoCommitListBuild(RefPtr<mozilla::widget::detail::DoneCommitListBuildCallback>) [JumpListBuilder.cpp:c5e8b08a4635 : 494 + 0x9]
13:21:41     INFO -     eip = 0x5d5bf093   esp = 0x12fff57c   ebp = 0x12fff584
13:21:41     INFO -     Found by: previous frame's frame pointer
13:21:41     INFO -  8  xul.dll!mozilla::detail::RunnableMethodArguments<RefPtr<mozilla::widget::detail::DoneCommitListBuildCallback> >::applyImpl<mozilla::widget::JumpListBuilder,void ( mozilla::widget::JumpListBuilder::*)(RefPtr<mozilla::widget::detail::DoneCommitListBuildCallback>),StoreRefPtrPassByPtr<mozilla::widget::detail::DoneCommitListBuildCallback>,0>(mozilla::widget::JumpListBuilder *,void ( mozilla::widget::JumpListBuilder::*)(RefPtr<mozilla::widget::detail::DoneCommitListBuildCallback>),mozilla::Tuple<StoreRefPtrPassByPtr<mozilla::widget::detail::DoneCommitListBuildCallback> > &,mozilla::IndexSequence<0>) [nsThreadUtils.h:c5e8b08a4635 : 1131 + 0x1d]
13:21:41     INFO -     eip = 0x5d5be8db   esp = 0x12fff58c   ebp = 0x12fff590
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO -  9  xul.dll!mozilla::detail::RunnableMethodImpl<mozilla::widget::JumpListBuilder * const,void ( mozilla::widget::JumpListBuilder::*)(RefPtr<mozilla::widget::detail::DoneCommitListBuildCallback>),0,0,RefPtr<mozilla::widget::detail::DoneCommitListBuildCallback> >::Run() [nsThreadUtils.h:c5e8b08a4635 : 1180 + 0x15]
13:21:41     INFO -     eip = 0x5d5bf6b7   esp = 0x12fff598   ebp = 0x12fff5ac
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 10  xul.dll!nsThread::ProcessNextEvent(bool,bool *) [nsThread.cpp:c5e8b08a4635 : 1421 + 0x6]
13:21:41     INFO -     eip = 0x5c5f7243   esp = 0x12fff5b4   ebp = 0x12fff6b8
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 11  nss3.dll!_MD_CURRENT_THREAD [w95thred.c:c5e8b08a4635 : 317 + 0xc]
13:21:41     INFO -     eip = 0x724cfa10   esp = 0x12fff5d0   ebp = 0x12fff6b8
13:21:41     INFO -     Found by: stack scanning
13:21:41     INFO - 12  xul.dll!nsThreadManager::GetCurrentThread() [nsThreadManager.cpp:c5e8b08a4635 : 234 + 0x9]
13:21:41     INFO -     eip = 0x5c5f4cb7   esp = 0x12fff5e0   ebp = 0x12fff6b8
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 13  xul.dll!nsThreadManager::GetCurrentThread(nsIThread * *) [nsThreadManager.cpp:c5e8b08a4635 : 337 + 0x6]
13:21:41     INFO -     eip = 0x5c5f4d41   esp = 0x12fff5e8   ebp = 0x12fff6b8
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 14  xul.dll!NS_ProcessNextEvent(nsIThread *,bool) [nsThreadUtils.cpp:c5e8b08a4635 : 471 + 0xd]
13:21:41     INFO -     eip = 0x5c5f69b8   esp = 0x12fff6c0   ebp = 0x12fff6d4
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 15  xul.dll!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate *) [MessagePump.cpp:c5e8b08a4635 : 338 + 0x8]
13:21:41     INFO -     eip = 0x5c88b1cc   esp = 0x12fff6dc   ebp = 0x12fff6fc
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 16  xul.dll!MessageLoop::RunHandler() [message_loop.cc:c5e8b08a4635 : 313 + 0x8]
13:21:41     INFO -     eip = 0x5c875a97   esp = 0x12fff704   ebp = 0x12fff734
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 17  xul.dll!MessageLoop::Run() [message_loop.cc:c5e8b08a4635 : 293 + 0x7]
13:21:41     INFO -     eip = 0x5c87584d   esp = 0x12fff73c   ebp = 0x12fff754
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 18  xul.dll!nsThread::ThreadFunc(void *) [nsThread.cpp:c5e8b08a4635 : 503 + 0x7]
13:21:41     INFO -     eip = 0x5c5f94c8   esp = 0x12fff75c   ebp = 0x12fff778
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 19  nss3.dll!_PR_NativeRunThread [pruthr.c:c5e8b08a4635 : 397 + 0x6]
13:21:41     INFO -     eip = 0x724da6a8   esp = 0x12fff780   ebp = 0x12fff794
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 20  nss3.dll!pr_root [w95thred.c:c5e8b08a4635 : 95 + 0xa]
13:21:41     INFO -     eip = 0x724cfcbe   esp = 0x12fff79c   ebp = 0x12fff7a0
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 21  ucrtbase.dll!_o__CIpow + 0x4f
13:21:41     INFO -     eip = 0x7272d5ef   esp = 0x12fff7a8   ebp = 0x12fff7dc
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 22  kernel32.dll!BaseThreadInitThunk + 0x12
13:21:41     INFO -     eip = 0x76bc3c45   esp = 0x12fff7e4   ebp = 0x12fff7e8
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 23  mozglue.dll!patched_BaseThreadInitThunk [WindowsDllBlocklist.cpp:c5e8b08a4635 : 847 + 0x15]
13:21:41     INFO -     eip = 0x7237265f   esp = 0x12fff7f0   ebp = 0x12fff81c
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 24  ntdll.dll!__RtlUserThreadStart + 0x27
13:21:41     INFO -     eip = 0x779b37f5   esp = 0x12fff824   ebp = 0x12fff85c
13:21:41     INFO -     Found by: call frame info
13:21:41     INFO - 25  ntdll.dll!_RtlUserThreadStart + 0x1b
13:21:41     INFO -     eip = 0x779b37c8   esp = 0x12fff864   ebp = 0x12fff874
13:21:41     INFO -     Found by: call frame info

Comment 23

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Tried on a Windows 10 machine but didn't reproduce this crash. Probably Windows 7 only?

aCallback should holds a strong reference to the JumpListBuilder, and it passed those assertion, so I guess at least those pointers are valid during the call.
Maybe ICDestinationList is not usable at the time, e.g. shutting down the whole application?

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

After extremely helpful discussions with :gabor and :mconley, a new plan has emerged.  We are no longer going to attempt to back this out at all, and will instead leave about:newtab in a content process.  Thanks for all the help, y'all!