This bug was filed from the Socorro interface and is report bp-68d18789-93f6-4eeb-a830-5be2f2161109. ============================================================= mozilla::dom::FontFaceSet::StartLoad() can call mozilla::net::PredictorLearn(). This in turn can call IPC::SerializedLoadContext::Init(), I think due to bug Bug 1304219. This ends up calling nsILoadContext::GetOriginAttributes, which ends up in the JS engine. If this happens during painting, then we hit a fatal assert.

I don't think this is related to the Bug 1304219 since it will call nsILoadContext::GetOriginAttributes() even before the Bug 1304219.

Nick, do you have time to take a look?

probably someone from the security group is a better person to figure out originAttributes.

Sorry, that got mangled. The crash under this circumstance goes back as far as 51 (from bug 1279086 and deps), so calling this a regression from that.

Maybe baku knows about the originAttributes aspect here and can help with some insight?

I think this can be closed now. It no longer applies after bug 1328423.

I'd like to try to revive the paint during GC project. :-) Taking this one.

(In reply to :Ehsan Akhgari from comment #8) > I'd like to try to revive the paint during GC project. :-) Taking this one. Why?

(In reply to Bill McCloskey (:billm) from comment #9) > (In reply to :Ehsan Akhgari from comment #8) > > I'd like to try to revive the paint during GC project. :-) Taking this one. > > Why? I'd like to try to extend the coverage of APZ, and one of the aspects that I'm looking at is reducing checkerboarding. Our GC times are quite terrible, and while I'm hoping that we can work on improving both our GC times, and their scheduling, I'd like to have this as a band-aid of sorts for scrolling. I've looked at the bugs that were left the last time we decided to back this out and it seems that with some work we should be able to fix them, so I'd like to try it out. (Also, this bug on its own is valuable, see bug 1348460.)

Our caller is C++ code, and the implementations are all also written in C++, so there is no reason to go through SpiderMonkey here. This patch also makes nsILoadContext builtinclass to ensure that the implementation is always native.

(In reply to :Ehsan Akhgari from comment #10) > (In reply to Bill McCloskey (:billm) from comment #9) > > (In reply to :Ehsan Akhgari from comment #8) > > > I'd like to try to revive the paint during GC project. :-) Taking this one. > > > > Why? > > I'd like to try to extend the coverage of APZ, and one of the aspects that > I'm looking at is reducing checkerboarding. Extending the painting stuff to cover APZ seems like a good idea to me. There's some good information in bug 1305130 if you haven't seen it already. > Our GC times are quite terrible, and while I'm hoping that we can work on > improving both our GC times, and their scheduling, I'd like to have this as > a band-aid of sorts for scrolling. I've looked at the bugs that were left > the last time we decided to back this out and it seems that with some work > we should be able to fix them, so I'd like to try it out. We can do the APZ thing without fixing this GC stuff. I think doing APZ by itself would be a lot easier. And despite our not great GC times, there isn't a lot of evidence that GC contributes much to tab switching problems. Turning off the GC stuff didn't help any of the telemetry measurements that we have for tab switch times or spinner duration. And the stacks that Mike has for tab switching jank don't show GC to be a major factor (please ask him for the bug# that has the latest stacks, I can't find it). Given all the bugs with the GC stuff, this is not low hanging fruit. And it probably won't help much even if we fix it. So this just seems like a bad thing to work on to me. > (Also, this bug on its own is valuable, see bug 1348460.) Well, okay, that's fine.

Comment on attachment 8848792 [details] [diff] [review] Avoid going into SpiderMonkey for retrieving origin attributes Review of attachment 8848792 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsILoadContext.idl @@ +142,2 @@ > */ > + NS_IMETHOD_(bool) GetOriginAttributes(mozilla::OriginAttributes& aAttrs) = 0; No reasons to return bool. void is enough if you change TabParent. ::: dom/ipc/TabParent.cpp @@ +3012,5 @@ > NS_IMETHOD GetIsInIsolatedMozBrowserElement(bool*) NO_IMPL > NS_IMETHOD GetOriginAttributes(JS::MutableHandleValue) NO_IMPL > + NS_IMETHOD_(bool) GetOriginAttributes(mozilla::OriginAttributes&) override > + { > + return false; Also TabParent has an originAttributes. Just do: aAttrs = OriginAttributesRef(); return true;

(In reply to Andrea Marchesini [:baku] from comment #14) > Comment on attachment 8848792 [details] [diff] [review] > Avoid going into SpiderMonkey for retrieving origin attributes > > Review of attachment 8848792 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: docshell/base/nsILoadContext.idl > @@ +142,2 @@ > > */ > > + NS_IMETHOD_(bool) GetOriginAttributes(mozilla::OriginAttributes& aAttrs) = 0; > > No reasons to return bool. void is enough if you change TabParent. OK. > ::: dom/ipc/TabParent.cpp > @@ +3012,5 @@ > > NS_IMETHOD GetIsInIsolatedMozBrowserElement(bool*) NO_IMPL > > NS_IMETHOD GetOriginAttributes(JS::MutableHandleValue) NO_IMPL > > + NS_IMETHOD_(bool) GetOriginAttributes(mozilla::OriginAttributes&) override > > + { > > + return false; > > Also TabParent has an originAttributes. Just do: > > aAttrs = OriginAttributesRef(); > return true; This is FakeChannel, not TabParent. :-)

(In reply to Bill McCloskey (:billm) from comment #13) > We can do the APZ thing without fixing this GC stuff. I think doing APZ by > itself would be a lot easier. That's good to know, but I'd like to understand why better. I'll try to ping you on IRC today to understand why. > And despite our not great GC times, there isn't a lot of evidence that GC > contributes much to tab switching problems. Turning off the GC stuff didn't > help any of the telemetry measurements that we have for tab switch times or > spinner duration. And the stacks that Mike has for tab switching jank don't > show GC to be a major factor (please ask him for the bug# that has the > latest stacks, I can't find it). Sorry for the confusion, I wasn't really focusing on tab switches, I was (perhaps naively) assuming some of the bugs we hit with painting during GC when tab switching will happen when painting while GCing for APZ (which is of course the next step after we do bug 1305130.) Perhaps my assumption there was incorrect? I'll sync up with you on that.

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fecc6abcf708 Avoid going into SpiderMonkey for retrieving origin attributes; r=baku

Backed out for bustage at docshell/base/SerializedLoadContext.cpp:65:61: https://hg.mozilla.org/integration/mozilla-inbound/rev/2857884dea0e7c17972fdda9eca35928722ffd08 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fecc6abcf708eb09f5af40f50ad0a825842e43a1&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=85341354&repo=mozilla-inbound /home/worker/workspace/build/src/docshell/base/SerializedLoadContext.cpp:65:61: error: could not convert 'aLoadContext->nsILoadContext::GetOriginAttributes((* &((IPC::SerializedLoadContext*)this)->IPC::SerializedLoadContext::mOriginAttributes))' from 'void' to 'bool' /home/worker/workspace/build/src/docshell/base/SerializedLoadContext.cpp:65:61: error: in argument to unary !

Sorry, my local build was too slow, and I was too impatient. :(

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7e47807067a6 Avoid going into SpiderMonkey for retrieving origin attributes; r=baku

Backed out for Windows bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f8950030a5b600e29df14da687ef35b70a5ce5 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7e47807067a60a61a8cb205e240dbc679163b55b&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=85633193&repo=mozilla-inbound 14:12:46 INFO - z:\build\build\src\uriloader\prefetch\OfflineCacheUpdateParent.h(57): error C2695: 'mozilla::docshell::OfflineCacheUpdateParent::GetOriginAttributes': overriding virtual function differs from 'nsILoadContext::GetOriginAttributes' only by calling convention 14:12:46 INFO - z:\build\build\src\obj-firefox\dist\include\nsILoadContext.h(102): note: see declaration of 'nsILoadContext::GetOriginAttributes' 14:12:46 INFO - z:/build/build/src/uriloader/prefetch/OfflineCacheUpdateParent.cpp(280): error C2373: 'mozilla::docshell::OfflineCacheUpdateParent::GetOriginAttributes': redefinition; different type modifiers 14:12:46 INFO - z:\build\build\src\uriloader\prefetch\OfflineCacheUpdateParent.h(57): note: see declaration of 'mozilla::docshell::OfflineCacheUpdateParent::GetOriginAttributes' 14:12:46 INFO - z:/build/build/src/config/rules.mk:1011: recipe for target 'Unified_cpp_uriloader_prefetch0.obj' failed 14:12:46 INFO - mozmake.EXE[5]: *** [Unified_cpp_uriloader_prefetch0.obj] Error 2 14:12:46 INFO - mozmake.EXE[5]: Leaving directory 'z:/build/build/src/obj-firefox/uriloader/prefetch' 14:12:46 INFO - z:/build/build/src/config/recurse.mk:71: recipe for target 'uriloader/prefetch/target' failed 14:12:46 INFO - mozmake.EXE[4]: *** [uriloader/prefetch/target] Error 2

So sorry. :(

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ef22dba0dac5 Avoid going into SpiderMonkey for retrieving origin attributes; r=baku

Backed out for various test failures, e.g. xpcshell netwerk/test/unit/test_bug826063.js and browser-chrome browser/components/downloads/test/browser/browser_iframe_gone_mid_download.js: https://hg.mozilla.org/integration/mozilla-inbound/rev/8b30d5f5b423e6047eff9095c6f7181c331c8972 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ef22dba0dac560980462f670db732470d5e364c1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log xpcshell: https://treeherder.mozilla.org/logviewer.html#?job_id=85705269&repo=mozilla-inbound [task 2017-03-22T19:08:39.724603Z] 19:08:39 INFO - TEST-PASS | netwerk/test/unit/test_bug826063.js | test_setPrivate_regular - [test_setPrivate_regular : 38] false == false [task 2017-03-22T19:08:39.726328Z] 19:08:39 INFO - (xpcshell/head.js) | test run_next_test 3 pending (3) [task 2017-03-22T19:08:39.728018Z] 19:08:39 INFO - (xpcshell/head.js) | test test_setPrivate_regular finished (3) [task 2017-03-22T19:08:39.729713Z] 19:08:39 INFO - (xpcshell/head.js) | test run_next_test 2 finished (2) [task 2017-03-22T19:08:39.731474Z] 19:08:39 INFO - netwerk/test/unit/test_bug826063.js | Starting test_LoadContextPrivate [task 2017-03-22T19:08:39.733200Z] 19:08:39 INFO - (xpcshell/head.js) | test test_LoadContextPrivate pending (2) [task 2017-03-22T19:08:39.740884Z] 19:08:39 WARNING - TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_bug826063.js | test_LoadContextPrivate - [test_LoadContextPrivate : 38] false == true [task 2017-03-22T19:08:39.742842Z] 19:08:39 INFO - /home/worker/workspace/build/tests/xpcshell/tests/netwerk/test/unit/test_bug826063.js:checkPrivate:38 [task 2017-03-22T19:08:39.744677Z] 19:08:39 INFO - /home/worker/workspace/build/tests/xpcshell/tests/netwerk/test/unit/test_bug826063.js:test_LoadContextPrivate:82 [task 2017-03-22T19:08:39.746428Z] 19:08:39 INFO - /home/worker/workspace/build/tests/xpcshell/head.js:_run_next_test:1569 [task 2017-03-22T19:08:39.748141Z] 19:08:39 INFO - /home/worker/workspace/build/tests/xpcshell/head.js:run:703 [task 2017-03-22T19:08:39.749851Z] 19:08:39 INFO - /home/worker/workspace/build/tests/xpcshell/head.js:_do_main:211 [task 2017-03-22T19:08:39.751592Z] 19:08:39 INFO - /home/worker/workspace/build/tests/xpcshell/head.js:_execute_test:546 [task 2017-03-22T19:08:39.753240Z] 19:08:39 INFO - -e:null:1 [task 2017-03-22T19:08:39.754906Z] 19:08:39 INFO - exiting test Failure log browser-chrome: https://treeherder.mozilla.org/logviewer.html#?job_id=85705286&repo=mozilla-inbound [task 2017-03-22T19:03:32.135540Z] 19:03:32 INFO - TEST-START | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js [task 2017-03-22T19:03:34.260942Z] 19:03:34 INFO - TEST-INFO | started process screentopng [task 2017-03-22T19:03:35.478734Z] 19:03:35 INFO - TEST-INFO | screentopng: exit 0 [task 2017-03-22T19:03:35.482190Z] 19:03:35 INFO - Buffered messages logged at 19:03:34 [task 2017-03-22T19:03:35.485006Z] 19:03:35 INFO - TEST-PASS | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | downloadLastDir should be a valid object - [task 2017-03-22T19:03:35.489792Z] 19:03:35 INFO - TEST-PASS | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | LastDir pref should be null to start with - [task 2017-03-22T19:03:35.494381Z] 19:03:35 INFO - TEST-PASS | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | LastDir should point to the tmpDir - [task 2017-03-22T19:03:35.496805Z] 19:03:35 INFO - TEST-PASS | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | downloadLastDir.file should not be pointing to tmpDir - [task 2017-03-22T19:03:35.498808Z] 19:03:35 INFO - Buffered messages finished [task 2017-03-22T19:03:35.506482Z] 19:03:35 INFO - TEST-UNEXPECTED-FAIL | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | {} - [task 2017-03-22T19:03:35.509501Z] 19:03:35 INFO - Stack trace: [task 2017-03-22T19:03:35.512705Z] 19:03:35 INFO - chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js:test/<:10 [task 2017-03-22T19:04:17.210898Z] 19:04:17 INFO - Longer timeout required, waiting longer... Remaining timeouts: 1 [task 2017-03-22T19:05:02.212758Z] 19:05:02 INFO - Not taking screenshot here: see the one that was previously logged [task 2017-03-22T19:05:02.216642Z] 19:05:02 INFO - TEST-UNEXPECTED-FAIL | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | Test timed out - There are more failures, please verify with Try that these also got fixed. Thank you for your endurance.

So this completely blew up on inbound. Again. :( https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ef22dba0dac560980462f670db732470d5e364c1 For one thing we have this test which implements nsILoadContext. <http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/netwerk/test/unit/test_bug826063.js#22> As the reviewer of the test, I'd be fine with removing it at this point. What do you think, Andrea? Also, I'm not 100% sure if making this method void was the right idea. I suspect some of these bustages are the result of those changes (I admit I didn't push to try after addressing those changes.) See the places in the patch where I'm removing the checks for the return values. Can you please try to convince me why it's OK to make this method return void?

re: comment 25: I assume the issue is that we don't want to ever have a JS-implemented nsILoadContext? I looked and we've got 3 necko unit tests that implement nsILoadContext: test_predictor.js, test_cacheflags.js, and test_bug826063.js. Test_predictor is just implementing it because predictor.predict() takes one as an argument (does passing 'null' in JS result in null in the C++ code? If so we could just change the arg to null and the predictor C++ code will skip the check, which is the same as privateBrowsing=false, so no change to test logic/result). Test_cacheflags is using it to make sure we don't store into the cache during PB. That would be nice to have test coverage for, but if it's a pain to support it we can live w/o it. So yes, we can blow away all these test implementations of nsILoadContext as needed.

Nick, can you write a patch that changes the tests mentioned in comment 27 (sounds like test_bug826063.js will just go away? Remove refs to nsILoadContext from the other two), push it to try and see if anything still breaks? I'm trying to take some load off ehsan.

test_predictor doesn't create an nsILoadContext (I think it did in the past, but the change to use originAttributes got rid of that), so that one's fine. Seems like everyone's ok getting rid of test_bug826063, so that's easy. However, I worry about removing our test for not storing into the cache during PB. That seems like the kind of thing we would want to have tested. Like, I don't *think* we'd break it, but given we're all human... I don't trust us that far :) Would it be totally crazy to have some way to get a natively-implemented nsILoadContext that we could use in these tests? That would make me feel more comfortable than just dropping our testing of caching in PB mode.

I'd be fine with some kind of dummy nsLoadContext.cpp that our tests could grab from XPCOM somehow (and be able to set the privateBrowsing field). That class is mostly a stub anyway. But going back to comment 0, it looks like the existing code is calling GetOriginAttributes() on the loadContext, and I'm not sure what we'd hand back. We may need to sync with ehsan to figure out what to do with this. (meanwhile nick is going to do a try run with all the JS test nsILoadContexts removed and see what the results look like. Nick, you could also try making GetOriginAttributes not return void...).

Comment on attachment 8848792 [details] [diff] [review] Avoid going into SpiderMonkey for retrieving origin attributes Sorry for my delay here, the test issue I think is fairly easy to solve, we should be able to add an XPCOM constructor to the class in docshell/base/LoadContext.h and just use that in the test. I don't see any reason why that wouldn't work. But I'm still waiting for baku's response to the rest of comment 25. Let me try resetting the review request. :-)

Comment on attachment 8848792 [details] [diff] [review] Avoid going into SpiderMonkey for retrieving origin attributes Review of attachment 8848792 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay: 1. OK with removing that test. 2. I still think that we don't need to return a boolean. Each GetOriginAttributes() returns true (If you change TabParent). so, so far, there are no needs to return false and or to check the return value.

The patch changes the tests that create an nsILoadContext in script to use ones made natively. It's meant to apply on top of Ehsan's patch from above. Running through try now https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bd6d6b8b9ab84257ccd33e223033809a6177387 with both patches applied (though in the opposite order, 'cause I'm silly).

Comment on attachment 8855502 [details] Bug 1316683 - make nsILoadContext non-scriptable. https://reviewboard.mozilla.org/r/127352/#review130134 Thank you! ::: netwerk/test/unit/test_bug826063.js:74 (Diff revision 1) > * Load context mandates private mode > */ > add_test(function test_LoadContextPrivate() { > - let ctx = new LoadContext(true); > for (let c of getChannels()) { > - c.notificationCallbacks = ctx; > + c.notificationCallbacks = privateContext; This is changing the test slightly by reusing the same load context object. I suggest changing the above to something like: let LoadContext = Components.Constructor("@mozilla.org/loadcontext;1"); // similarly for PrivateLoadContext and just change the definition of ctx here and below accordingly.

(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #34) > The patch changes the tests that create an nsILoadContext in script to use > ones made natively. It's meant to apply on top of Ehsan's patch from above. > Running through try now > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=3bd6d6b8b9ab84257ccd33e223033809a6177387 with both > patches applied (though in the opposite order, 'cause I'm silly). There are still test failures. I need to look into why.

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #35) > Comment on attachment 8855502 [details] > Bug 1316683 - make nsILoadContext non-scriptable. > > https://reviewboard.mozilla.org/r/127352/#review130134 > > Thank you! > > ::: netwerk/test/unit/test_bug826063.js:74 > (Diff revision 1) > > * Load context mandates private mode > > */ > > add_test(function test_LoadContextPrivate() { > > - let ctx = new LoadContext(true); > > for (let c of getChannels()) { > > - c.notificationCallbacks = ctx; > > + c.notificationCallbacks = privateContext; > > This is changing the test slightly by reusing the same load context object. > I suggest changing the above to something like: > > let LoadContext = Components.Constructor("@mozilla.org/loadcontext;1"); > // similarly for PrivateLoadContext > > and just change the definition of ctx here and below accordingly. In order to simplify the landing process, I will address this comment myself when landing. :-)

This approach of declaring a pure virtual function doesn't work on Windows: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f93705dad773fa993d3d54324b2f977b3bdf8c2> For some reason, the xpt caller ends up calling that function, presumably the ordering of overloads is different. Let's move things to XPIDL and not rely on compiler magic!

This is the latest version version of the patch, and I think it has changed sufficiently to merit another review. Sorry for the additional requests on this bug.

Green on try: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=79087ca8bf33b24f13dab94c045757f93d14860b>

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bd78de6136de Part 1: Make nsILoadContext non-scriptable. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/1542d591be9f Part 2: Use the non-scriptable LoadContext object in the contentprefs module; r=hurley https://hg.mozilla.org/integration/mozilla-inbound/rev/5c2688db7d9d Part 3: Use the non-scriptable LoadContext object in DownloadLastDir.jsm; r=hurley https://hg.mozilla.org/integration/mozilla-inbound/rev/448f478547de Part 4: Use the non-scriptable LoadContext object in test_private_channel.js; r=hurley https://hg.mozilla.org/integration/mozilla-inbound/rev/adf1dbdf0042 Part 5: Avoid going into SpiderMonkey for retrieving origin attributes; r=baku

https://hg.mozilla.org/mozilla-central/rev/bd78de6136de https://hg.mozilla.org/mozilla-central/rev/1542d591be9f https://hg.mozilla.org/mozilla-central/rev/5c2688db7d9d https://hg.mozilla.org/mozilla-central/rev/448f478547de https://hg.mozilla.org/mozilla-central/rev/adf1dbdf0042