Closed Bug 820200 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(2 files, 4 obsolete files)

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
Assignee: nobody → jmathies
Attached patch disable test (obsolete) — Splinter Review
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
Attachment #691841 - Flags: review?(justin.lebar+bug)
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.
Attachment #691841 - Flags: review?(justin.lebar+bug) → review-
(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.
> 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.
FYI, the code which is responsible for firing this event lives in dom/browser-element/BrowserElementChild.js.
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!
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.
> assuming "other" is the correct type for an expired cert.

Yes, I think that's right.
Attached patch fix v.1 (obsolete) — Splinter Review
Working on Windows, need to do some testing on other platforms.
Attachment #691841 - Attachment is obsolete: true
Attached patch fix v.1 (obsolete) — Splinter Review
nits
Attachment #692034 - Attachment is obsolete: true
Attached patch fix v.2 (obsolete) — Splinter Review
Attachment #692037 - Attachment is obsolete: true
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?
Attachment #692261 - Flags: review?(justin.lebar+bug)
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.
Attached patch fix v.2Splinter Review
Sorry for the bug spam, cleaning up some nits in ScopedXREEmbed::SetAppDir.
Attachment #692261 - Attachment is obsolete: true
Attachment #692261 - Flags: review?(justin.lebar+bug)
Attachment #692266 - Flags: review?(justin.lebar+bug)
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.
Attachment #692262 - Flags: review?(justin.lebar+bug)
Blocks: 763081
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.
Attachment #692262 - Flags: review?(justin.lebar+bug) → review+
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.
Attachment #692266 - Flags: review?(justin.lebar+bug) → review?
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.
Attachment #692266 - Flags: review? → review?(benjamin)
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.
(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.
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.
Attachment #692266 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/3ec0a08450d8
https://hg.mozilla.org/mozilla-central/rev/8e79ce6ac505
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.