820200 - Mochitest-2 failures on mac and linux on elm: Assertion failure: failedURI (We don't have a URI for history APIs.)

Status: RESOLVED FIXED

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

Description

[Jim Mathies [:jimm]]

Likely app/gre splitting related:

Assertion failure: failedURI (We don't have a URI for history APIs.), at ../../../docshell/base/nsDocShell.cpp:7809
1296 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/browser-element/mochitest/test_browserElement_oop_ErrorSecurity.html | This test left crash dumps behind, but we weren't expecting it to!
1300 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/browser-element/mochitest/test_browserElement_oop_ExposableURI.html | Test timed out.
1301 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/browser-element/mochitest/test_browserElement_oop_ExposableURI.html | This test left crash dumps behind, but we weren't expecting it to!
2 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/devicestorage/test/test_addCorrectType.html | This test left crash dumps behind, but we weren't expecting it to!

https://tbpl.mozilla.org/php/getParsedLog.php?id=17797875&tree=Elm&full=1

Comment 1

[Jim Mathies [:jimm]]

Attachment: disable test

This test landed in bug 768842. It currently works on mc, but it won't for long. The test relies on about:certerror, which is provided by the browser. Once bug 755724 lands, app and gre resources will be split up. Content processes don't have access to app resources, so this test will fail, like it's currently doing on elm.

try run with elm + this change:

https://tbpl.mozilla.org/?tree=Try&rev=d68911969e31

Comment 2

[Justin Lebar (not reading bugmail)]

Do you know that the functionality that this test is testing works after bug 755724?

In any case, it's not OK to have untested browser-api features.  So we either need to fix the test, or fix the browser API.  But disabling the test is not an option.

Comment 3

[Jim Mathies [:jimm]]

(In reply to Justin Lebar [:jlebar] from comment #2)
> Do you know that the functionality that this test is testing works after bug
> 755724?
> 
> In any case, it's not OK to have untested browser-api features.  So we
> either need to fix the test, or fix the browser API.  But disabling the test
> is not an option.

mozbrowsererror does get delivered. 

I'm unsure of what this test is aimed at testing. Are content processes currently in use on any of our platforms? bsmedberg mentioned B2G might use these but that cert errors are only a concern at the top level.

Note the content process does crash from this missing certerror handler. So maybe that needs to be fixed, I'm not sure.

Comment 4

[Justin Lebar (not reading bugmail)]

> Are content processes currently in use on any of our platforms? 

Yes, extensively in B2G.  Every app runs in a content process, and each tab in the browser gets its own content process.

> but that cert errors are only a concern at the top level.

I'm pretty sure that the B2G browser needs to handle cert errors.  In fact, so do B2G apps.  So we need this code to work out of process.

Comment 5

[Justin Lebar (not reading bugmail)]

FYI, the code which is responsible for firing this event lives in dom/browser-element/BrowserElementChild.js.

Comment 6

[Jim Mathies [:jimm]]

Ok, so it sounds like the content process needs to be aware of app resources, which was my original guess. That shouldn't be too hard to do. Thanks for the feedback!

Comment 7

[Jim Mathies [:jimm]]

BTW your event seems to be working just fine for both errors -

TEST-PASS | unknown test url | Got mozbrowsererror event.
TEST-PASS | unknown test url | Event's detail has a |type| param.
------------------------------------> type: other
TEST-PASS | unknown test url | Got mozbrowsererror event.
TEST-PASS | unknown test url | Event's detail has a |type| param.
------------------------------------> type: fatal

assuming "other" is the correct type for an expired cert.

Comment 8

[Justin Lebar (not reading bugmail)]

> assuming "other" is the correct type for an expired cert.

Yes, I think that's right.

Comment 9

[Jim Mathies [:jimm]]

Attachment: fix v.1

Working on Windows, need to do some testing on other platforms.

Comment 10

[Jim Mathies [:jimm]]

Attachment: fix v.1

nits

Comment 11

[Jim Mathies [:jimm]]

Attachment: fix v.2

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 692261 [details] [diff] [review]
fix v.2

This patch passes the application path down to content processes. This passes on try fine on win, mac, and linux with oop tests enabled.

Justin, are you cool with reviewing this or should I bug bsmedberg/cjones?

Comment 13

[Jim Mathies [:jimm]]

Attachment: enable oop tests on Win

I'm running a few try runs looking for intermittent problems. If I don't see anything I'll seek review on this as well.

Comment 14

[Jim Mathies [:jimm]]

Attachment: fix v.2

Sorry for the bug spam, cleaning up some nits in ScopedXREEmbed::SetAppDir.

Comment 15

[Jim Mathies [:jimm]]

Comment on attachment 692262 [details] [diff] [review]
enable oop tests on Win

These are passing fine on try and elm. Might as well enable them.

Comment 16

[Justin Lebar (not reading bugmail)]

Comment on attachment 692262 [details] [diff] [review]
enable oop tests on Win

If this works, that's great!  But since we don't ship content processes on Windows, I wouldn't get bent out of shape if this runs into trouble when landing.

Comment 17

[Justin Lebar (not reading bugmail)]

Comment on attachment 692266 [details] [diff] [review]
fix v.2

I don't know what's going on here or why this fixes the issue; I'd prefer if you asked bsmedberg or cjones.

Comment 18

[Jim Mathies [:jimm]]

Comment on attachment 692266 [details] [diff] [review]
fix v.2

no problem. Since I've already chatted with bsmedberg about this, switching the review over to him.

Benjamin, we determined that content processes do want access to app resources. This patch fixes up how they launch such that we can pass the app path to XRE_InitEmbedding2.

Comment 19

[Benjamin Smedberg]

How/where did we come to the conclusion that we want app components? That's a pretty major change and could have widespread consequences where we're loading code that we previously weren't.

Comment 20

[Jim Mathies [:jimm]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #19)
> How/where did we come to the conclusion that we want app components? That's
> a pretty major change and could have widespread consequences where we're
> loading code that we previously weren't.

Content processes have always had access to app resources since the two are mixed into the same set of directories. (I'm assuming here B2G is no different than desktop in this respect. Removing app caused at least one known issue, the crash here where an about handler provided by the browser front end wasn't present for a test. I imagine the same crash would show up in a content process tab once metro-build lands. There could be other hidden issues we haven't hit yet.

Comment 21

[Jim Mathies [:jimm]]

B2G won't be affected by this since we won't be adding the directive that breaks app and gre up in that build. browser is the only project that will and there, we have tests for content processes that assume app and gre are both available. So I think this change is safe to take.

Comment 22

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3ec0a08450d8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8e79ce6ac505

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ec0a08450d8
https://hg.mozilla.org/mozilla-central/rev/8e79ce6ac505