The first stage of removing the platform code to support overlays: 1) crash on loading overlays 2) remove all overlay tests

We're busily removing overlays from Thunderbird, but it might take us another week before we're done. Perhaps you could hide the crash behind a preference so we get a bit more time to complete the work.

(In reply to Jorg K (GMT+1) from comment #1) > We're busily removing overlays from Thunderbird, but it might take us > another week before we're done. Perhaps you could hide the crash behind a > preference so we get a bit more time to complete the work. That or I can use MOZ_BUILD_APP. Do you have a meta bug tracking XUL overlay removal in Thunderbird?

Yes, bug 1444468. I need to link a few we have removed there.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4f1ba8b85d492b6ca2cffca9768f3de5da5dd84 I tried to be as thorough as possible, but there were quite a few things to remove.

Comment on attachment 8963429 [details] Bug 1448162 - Remove all XUL overlay tests. https://reviewboard.mozilla.org/r/232330/#review237928 ::: commit-message-de322:1 (Diff revision 1) > +BUg 1448162 - Remove all XUL overlay tests. r?gijs Nitty nit: "Bug" ::: chrome/test/unit/test_bug564667.js (Diff revision 1) > - // Test Adding Override > - test_mapping("chrome://testOverride/content", "file:///test1/override"); This looks like a test for a chrome url override, not a XUL overlay? So it should probably be kept. ::: layout/reftests/xul-document-load/readme.txt (Diff revision 1) > -test010: PIs in the master document, outside the prolog, don't have any effect but get > - added to the DOM. This test looks to me like it ought not to be deleted, but have the overlay file removed, and the overlay reference removed, and the overlay checks in the test removed -- but the stylesheet PI checks should be kept as they continue to be correct and reasonable (AFAICT). ::: layout/reftests/xul-document-load/readme.txt:24 (Diff revision 1) > test014: (bug 363406) Relative URIs in overlay's <?xul-overlay ?> PI are resolved > against overlay's URI, not the document URI. Looks like we should remove the mention of this test (which is correctly being removed), too?

Gah, forgot to point this out in my topline review comment: I'm not technically a dom/xul peer. So I think you need review from someone else...

Nika is probably a better reviewer for the crash patch, possibly also to check over the test removal.

Comment on attachment 8963430 [details] Bug 1448162 - Crash in the presence of a XUL overlay. https://reviewboard.mozilla.org/r/232332/#review238932 r=me with the changes I requested ::: dom/xul/XULDocument.cpp:2262 (Diff revision 3) > bool* aShouldReturn, > bool* aFailureFromContent) > { > nsresult rv; > > +#ifdef MOZ_BUILD_APP_IS_BROWSER Please add a comment referencing this bug and explaining why we enable this based on what MOZ_BUILD_APP is. ::: dom/xul/XULDocument.cpp:2268 (Diff revision 3) > + nsCString docSpec; > + mCurrentPrototype->GetURI()->GetSpec(docSpec); > + nsCString overlaySpec; > + aURI->GetSpec(overlaySpec); > + printf("Attempt to load overlay %s into %s\n", > + PromiseFlatCString(overlaySpec).get(), PromiseFlatCString is completely unnecessary here and below, because nsCString is already flat (and thus exposes the .get() method). ::: dom/xul/moz.build:11 (Diff revision 3) > > with Files("**"): > BUG_COMPONENT = ("Core", "XUL") > > +if CONFIG['MOZ_BUILD_APP'] == 'browser': > + DEFINES['MOZ_BUILD_APP_IS_BROWSER'] = True This seems like an odd define to make locally in dom/xul... How about we call it `MOZ_CRASH_XUL_OVERLAYS` instead?

Thanks for putting this behind an ifdef! I'm sorry if I missed this, but even without Thunderbird, what is the rationale behind crashing in this case? Wouldn't it make more sense to show errors or warnings, instead of crashing the whole product?

Or you could if on the MOZ_THUNDERBIRD (and MOZ_SUITE) define.

(In reply to Philipp Kewisch [:Fallen] from comment #15) > Thanks for putting this behind an ifdef! I'm sorry if I missed this, but > even without Thunderbird, what is the rationale behind crashing in this > case? Wouldn't it make more sense to show errors or warnings, instead of > crashing the whole product? In Firefox I want to ensure no one adds any new overlays. An error or warning would probably be okay, but they often go unnoticed.

This is ideally something to be caught during a review. This is just my opinion, but I think a crash is a bit harsh. I probably would have gone with an error message and then just not loading the overlay. Anyway, given this code after the crash will likely go away very soon now it is probably not worth discussing the exact approach. Thanks again!

Pushed by bdahl@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ee4c52cd9d53 Remove all XUL overlay tests. r=Gijs https://hg.mozilla.org/integration/autoland/rev/61c57ad4f4f4 Crash in the presence of a XUL overlay. r=mystor

https://hg.mozilla.org/mozilla-central/rev/ee4c52cd9d53 https://hg.mozilla.org/mozilla-central/rev/61c57ad4f4f4