Closed Bug 1471327 Opened 6 years ago Closed 4 years ago

Stop touching `content` in browser/base/content/tab-content.js

Categories

(Firefox :: General, enhancement, P2)

Product:
Firefox
Component:
General
Version:
57 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
90 Branch
Tracking Flags:
Tracking Status
firefox90 --- fixed

People

(Reporter: kmag, Assigned: nika)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p3])

Attachments

(1 file)

Bug 1471327 - Eagerly create initial about:blank document for in-process browsers,
(deleted), text/x-phabricator-request
Details
When I lazified the loading of PluginContent.jsm in bug 1470023, some tests that depended on us touching `content` there started failing. Touching `content` early forces us to create an about:blank content viewer, which is relatively expensive. We shouldn't do it if we don't need to.
On my local testing, it looks like removing this will yield a >5% cpstartup improvement. When we remove this, I think it will be very important to add a test to make sure this is not introduced again, because this is something very easy to do without noticing.
Blocks: cpstartup-perf
Whiteboard: [fxperf]
According to this try run [1], the failures that show up with the removal of that line are: test-oop-extensions/ - test_ext_contentscript_permission.html - test_ext_redirect_jar.html - test_ext_webrequest_hsts.html [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d406a52ac68183facc833642b80e824513258ed
Shane, can you take a look at the redirect_jar and hsts failures? I looked at the latter, but didn't get very far. They seem to be depending on us creating an empty content viewer super early, for some reason. I can take care of the contentscript_permissions one.
Flags: needinfo?(mixedpuppy)
In both cases when we do not touch content, code executes early enough that it is getting an about:blank document that we did not get when touching content. hsts: This test uses tabs.onUpdated to wait for a "complete" update to close the tab. When we do not touch content, we get a *new* early event for the tab where tabInfo is: {"status":"complete","url":"about:blank"}. The fix for this to make this change: async function onUpdated(tabId, tabInfo, tab) { - if (tabInfo.status !== "complete") { + if (tabInfo.status !== "complete" || (tabInfo.url && tabInfo.url !== expectedUrl)) { return; } redirect_jar: This test calls tabs.create then immediately calls tabs.executeScript. The failure ultimately happens in ExtensionContent -> handleExtensionExecute. When we enumerate the windows, if we touched content, I see the url for the page I expect. If we do not, I see about:blank (and it does not pass the script.matchesWindow call). This test could probably be fixed by using an onUpdated listener (that is similar to the above fix) to wait before calling executeScript. In both cases, I worry we're going to break a bunch of extensions.
Flags: needinfo?(mixedpuppy)
Even though the contentscript_permissions test does the same thing as the redirect_jar test (create/executeScript), I cannot make it fail locally.
(In reply to :Felipe Gomes (needinfo me!) [offline Jul 6 - Jul 15] from comment #1) > On my local testing, it looks like removing this will yield a >5% cpstartup > improvement. fxperf:p2!
Whiteboard: [fxperf] → [fxperf:p2]
Priority: -- → P3
I've been on-and-off trying to debug the root cause for this and I got a pretty good lead (or thought so). It didn't seem enough solve the problem, but maybe it will ring a bell to someone else. (this was done experimenting with test_ext_redirect_jar.html) My theory was that it's caused by the fact that some oop-extensions code is initialized by a somewhat misleading notification, "tab-content-frameloader-created", which is actually dispatched by the front-end and not the backend. So the timing doesn't hold true anymore with the removal of `void content` from content.js For reference, the order in that the four main frame-scripts are loaded is: browser-content.js tab-content.js content.js browser-child.js However: - `notifyObservers(.., "tab-content-frameloader-created")` is in tab-content.js - `void content` is in content.js so the notifyObservers call was already before we touched content. I tried to move the notifyObservers to the end of browser-child.js or even on a timeout, but it didn't fix the problem, as expected. And there's also a separate thing that invokes that same notification, [1], which I'm not sure how it interacts here This is where I'm at right now.. Hopefully it's not a red herring.. If anyone has other suggestions on what to look for, let me know [1] https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/toolkit/components/extensions/ExtensionParent.jsm#525
My guess is that touching content is causing the about:blank content viewer to be created early and we're expecting it to already be created rather than waiting on some post-about:blank content viewer creation event. It makes me wonder if we need something akin to https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/browser/base/content/tab-content.js#513-519 but I have to wrap my head around the failing tests again.
Flags: needinfo?(mconley)
Flags: needinfo?(mconley)
Blocks: 1493606
Blocks: 1493634
I think I messed up creating bug 1493634, this bug shouldn't block that one.
No longer blocks: 1493634
Not sure what the current state is after all the other work, so I figured I'd just push to try fresh and see what happens: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fc5d7df08e3c59e1969e309c63fe699c69eeb73
(In reply to :Gijs (he/him) from comment #10) > Not sure what the current state is after all the other work, so I figured > I'd just push to try fresh and see what happens: > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=1fc5d7df08e3c59e1969e309c63fe699c69eeb73 This doesn't look great, but not completely terrible either. Almost all of these look like web extension issues, except bug521216.js which looks to me like we could just update the test expectation and move on - it's meant to guarantee TabOpen fires before everything else, not anything about web progress listener ordering (though it might be good to understand better why that change in ordering happens and if it should worry us). The previous comments implied concerns about the webextension compat side of things here. Does that concern still exist? Would things be better if we removed *all* the consumers that caused about:blank to be created here, thus ensuring that the same listeners *wouldn't* fire again? And/or, if we added code to suppress the onLocationChange for the initial about:blank document (which we already do in the tabbrowser.js code) ?
Flags: needinfo?(mixedpuppy)
FWIW, here's another try push that removes all the other code that causes us to hit createAboutBlankContentViewer for new browser window creation (for the content window in that new browser window). I'm not sure if it'll break more or less - only one way to find out, I guess: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a3c915e1edb098c63a2bbf5f25770b738e6dddd
(In reply to :Gijs (he/him) from comment #12) > FWIW, here's another try push that removes all the other code that causes us > to hit createAboutBlankContentViewer for new browser window creation (for > the content window in that new browser window). I'm not sure if it'll break > more or less - only one way to find out, I guess: > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=1a3c915e1edb098c63a2bbf5f25770b738e6dddd Uh, OK, so AsyncTabSwitcher doesn't like being passed browsers that don't have anything loaded, and there's a couple of other browser.currentURI accesses that don't like being null. I think the main culprit here is that we fire onLocationChange on the tabbrowser when we switch to a tab, which might not have loaded content yet, which might therefore have no currentURI, which updates all kinds of UI and makes things sad. Just wallpapering a bit for now, will think more about the right solution later: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1dc31d3db0566ff11ba400221912c48dee90626
Depends on: 1505850
The impact of this on startup should be minimal/nonexistent, so going to downgrade this to fxperf:p3 (please correct me if you think I'm wrong!). I've been looking into this for a while now. There's a few things from that investigation that I'll land in separate bugs because they seem strict improvements on the status quo, but fixing this particular bug is quite tricky. The main reason is that we attach the main tabbrowser webprogress listeners after the current `content` read. Right now, the read referenced for this bug forces the about:blank creation *before* those listeners are attached. Unfortunately, when we remove that access, the webprogress listeners get invoked once the initial about:blank *does* load (ie a bit later), and get passed a browser that, at that point, doesn't necessarily have anything loaded. This means `currentURI` is null, and a bunch of code breaks when that assumption is violated. These loads are also what breaks the relevant webextension tests mentioned in earlier comments, and they would be exposed to webextensions unless we avoid sending those notifications either from tabbrowser or from the webextension tabprogresslistener->webextension tab listener interfaces. There are 2 ways of fixing this: one is to try to avoid the about:blank document loading at all. This doesn't seem to be very workable in practice, as there are quite a number of different ways in which the document can be created - some through JS, some through platform C++ code - and in some cases this creation is necessary. As a semi-random example, focusing the browser in the parent process attempts to focus the content of the browser, which trips creating a content document in that browser, etc. The other is to try to get the tab progress listeners to ignore cases where they get notified for the initial about:blank load, but that runs the risk of it ignoring loads that it shouldn't be ignoring (like for js: URIs in a new tab). So that gets messy. There are also issues with docshell swapping (ie moving tabs to new windows), which currently assumes the docshells both have frameloaders. If the about:blank document hasn't been initialized and the tab is in the parent process, there is no frameloader, the swap fails and a bunch of tests break. This is sort of interesting in that, when moving a tab to a new window, really we will just destroy the other frameloader, so creating it is pretty pointless. But fixing that involves some DOM fu, and there's no point if we don't also fix this bug (which would initialize the document anyway), and I'm not 100% sure if it affects remote docshells or not (it definitely affects parent process ones), and in any case I expect very few of our users tear off tabs, and considering we need to create a parent process window and run initialization code in it, the costs of the about:blank docshell are probably fairly negligible by comparison. Then there are some issues with the async tab switcher. There's some trivial currentURI accesses from logging code that are easily fixed, but even with that addressed it gets confused. Specifically, it doesn't determine the tab switch is 'done' and doesn't send a tabswitchdone event, because it waits for layers to be there for the about-blank-less browser, which doesn't happen if we avoid creating about:blank and just try to load a http page in the browser that doesn't respond (e.g. as done in browser_urlbar_stop_pending.js ). FWIW, I e-mailed a few people about this issue, and so far I haven't had any responses indicating that people still believe fixing this is likely to be a major perf improvement, so unless that changes I won't be doing any further work on it.
Flags: needinfo?(mixedpuppy)
Whiteboard: [fxperf:p2] → [fxperf:p3]
(In reply to :Gijs (he/him) from comment #14) > The impact of this on startup should be minimal/nonexistent, so going to > downgrade this to fxperf:p3 (please correct me if you think I'm wrong!). I'm not sure how true that is. We generally try to reuse initial about:blank contentviewers for subsequent page loads, when we can, so some of the overhead of creating them is obviated. But we still fire extra observers that trigger expensive JS code when we create them, and there are a bunch of places where we create them where they can't be reused. I'm not sure how much of a perf win it would be in the end, though. > These loads are also what breaks the relevant webextension tests mentioned > in earlier comments, and they would be exposed to webextensions unless we > avoid sending those notifications either from tabbrowser or from the > webextension tabprogresslistener->webextension tab listener interfaces. I suspect there may be some performance win from not sending those notifications... > The other is to try to get the tab progress listeners to ignore cases where > they get notified for the initial about:blank load, but that runs the risk > of it ignoring loads that it shouldn't be ignoring (like for js: URIs in a > new tab). So that gets messy. So, there are essentially two different kinds of about:blank loads: 1) Initial about:blank contentViewer creations that are going to be replaced with a real document. These have document.readyState == "uninitialized", and should generally be ignored. And, 2) real about:blank document loads, either directly loaded, or implicitly loaded for the sake of running things like javascript: URIs. These have document.readyState == "loading", "interactive", or "complete". Telling them apart can be tricky, but we already have to do it for the sake of extension content scripts, so it's not really much trouble to do the same in other cases. > There are also issues with docshell swapping (ie moving tabs to new > windows), which currently assumes the docshells both have frameloaders. This is a rare enough corner case that I don't think it's worth worrying about. We can just force creating a content viewer if it makes things easier.
Priority: P3 → P2
(In reply to Kris Maglione [:kmag] from comment #15) > (In reply to :Gijs (he/him) from comment #14) > > The impact of this on startup should be minimal/nonexistent, so going to > > downgrade this to fxperf:p3 (please correct me if you think I'm wrong!). > > I'm not sure how true that is. We generally try to reuse initial about:blank > contentviewers for subsequent page loads, when we can, so some of the > overhead of creating them is obviated. But we still fire extra observers > that trigger expensive JS code when we create them, and there are a bunch of > places where we create them where they can't be reused. Can you elaborate on the second point? > I'm not sure how much of a perf win it would be in the end, though. > > > These loads are also what breaks the relevant webextension tests mentioned > > in earlier comments, and they would be exposed to webextensions unless we > > avoid sending those notifications either from tabbrowser or from the > > webextension tabprogresslistener->webextension tab listener interfaces. > > I suspect there may be some performance win from not sending those > notifications... We don't send them to JS today, because the document is created before we attach the web progress listener, so I don't think there are serious wins to be had here (from creating the doc later and then not sending the notifications). There may still be other (e.g. observer service) notifications that we can avoid sending, I guess, but I've never seen these show up in profiles so again, I'm skeptical how much that'd net us. Am I misunderstanding your point? > > The other is to try to get the tab progress listeners to ignore cases where > > they get notified for the initial about:blank load, but that runs the risk > > of it ignoring loads that it shouldn't be ignoring (like for js: URIs in a > > new tab). So that gets messy. > > So, there are essentially two different kinds of about:blank loads: 1) > Initial about:blank contentViewer creations that are going to be replaced > with a real document. These have document.readyState == "uninitialized", and > should generally be ignored. And, 2) real about:blank document loads, either > directly loaded, or implicitly loaded for the sake of running things like > javascript: URIs. These have document.readyState == "loading", > "interactive", or "complete". I don't think these are distinguishable as you suggest. First, because for the initial onStateChange (STATE_START) notification(s?), there isn't yet a document to check. Second, because per bug 1505850, I think we create an initial document of 'type' 1 in your list for js loads in new tabs, not type 2 (at least, we call EnsureContentViewer, so I don't understand how a different document would result from the same call). > Telling them apart can be tricky, but we already have to do it for the sake > of extension content scripts, so it's not really much trouble to do the same > in other cases. Can you point me to the relevant code? > > There are also issues with docshell swapping (ie moving tabs to new > > windows), which currently assumes the docshells both have frameloaders. > > This is a rare enough corner case that I don't think it's worth worrying > about. We can just force creating a content viewer if it makes things easier. Sure, and that's exactly what I did locally. I guess it's just one item in a long list of stuff to update if we really want to allow having browser elements and/or docshells hanging around without anything loaded in them. Our code makes lots of assumptions that just accessing the document is OK, and/or that currentURI is always non-null. Right now I'm just not convinced that all the added complexity of doing that is worth it.
Flags: needinfo?(kmaglione+bmo)
(In reply to :Gijs (he/him) from comment #16) > (In reply to Kris Maglione [:kmag] from comment #15) > > (In reply to :Gijs (he/him) from comment #14) > > > The impact of this on startup should be minimal/nonexistent, so going to > > > downgrade this to fxperf:p3 (please correct me if you think I'm wrong!). > > > > I'm not sure how true that is. We generally try to reuse initial about:blank > > contentviewers for subsequent page loads, when we can, so some of the > > overhead of creating them is obviated. But we still fire extra observers > > that trigger expensive JS code when we create them, and there are a bunch of > > places where we create them where they can't be reused. > > Can you elaborate on the second point? Which one? The one where the content viewer can't be reused? That's typically where we need to do a remoteness switch, but there are other odd corner cases that I can't remember off the top of my head. > > I suspect there may be some performance win from not sending those > > notifications... > > We don't send them to JS today, because the document is created before we > attach the web progress listener, so I don't think there are serious wins to > be had here (from creating the doc later and then not sending the > notifications). There may still be other (e.g. observer service) > notifications that we can avoid sending, I guess, but I've never seen these > show up in profiles so again, I'm skeptical how much that'd net us. Am I > misunderstanding your point? I've frequently seen them show up in profiles... Typically, the first piece of code that accidentally creates a contentViewer shows up as taking a huge chunk of time because of all of the other JS code the content viewer triggers. > > So, there are essentially two different kinds of about:blank loads: 1) > > Initial about:blank contentViewer creations that are going to be replaced > > with a real document. These have document.readyState == "uninitialized", and > > should generally be ignored. And, 2) real about:blank document loads, either > > directly loaded, or implicitly loaded for the sake of running things like > > javascript: URIs. These have document.readyState == "loading", > > "interactive", or "complete". > > I don't think these are distinguishable as you suggest. First, because for > the initial onStateChange (STATE_START) notification(s?), there isn't yet a > document to check. Second, because per bug 1505850, I think we create an > initial document of 'type' 1 in your list for js loads in new tabs, not type > 2 (at least, we call EnsureContentViewer, so I don't understand how a > different document would result from the same call). The type 1 documents typically get reused to load a subsequent document. But, regardless, when they're in the uninitialized state, we really want to avoid touching them as much as possible. They tend to behave in really unexpected ways. > > Telling them apart can be tricky, but we already have to do it for the sake > > of extension content scripts, so it's not really much trouble to do the same > > in other cases. > > Can you point me to the relevant code? https://searchfox.org/mozilla-central/rev/adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/toolkit/components/extensions/ExtensionPolicyService.cpp#480-493 > Our code makes lots of assumptions that just accessing the document is OK, > and/or that currentURI is always non-null. Right now I'm just not convinced > that all the added complexity of doing that is worth it. I know. And I think that we should generally try to reduce those assumptions as much as possible, because touching uninitialized about:blank content viewers generally doesn't do what people expect. And the fact that they tend to get reused to load other documents in arcane and counter-intuitive ways means that someone assuming that they really are about:blank documents early on can potentially have serious consequences.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #17) > (In reply to :Gijs (he/him) from comment #16) > > (In reply to Kris Maglione [:kmag] from comment #15) > > > (In reply to :Gijs (he/him) from comment #14) > > > > The impact of this on startup should be minimal/nonexistent, so going to > > > > downgrade this to fxperf:p3 (please correct me if you think I'm wrong!). > > > > > > I'm not sure how true that is. We generally try to reuse initial about:blank > > > contentviewers for subsequent page loads, when we can, so some of the > > > overhead of creating them is obviated. But we still fire extra observers > > > that trigger expensive JS code when we create them, and there are a bunch of > > > places where we create them where they can't be reused. > > > > Can you elaborate on the second point? > > Which one? The one where the content viewer can't be reused? Yep. > That's typically where we need to do a remoteness switch, FWIW, this shouldn't be happening to any tabbrowser tabs anymore. That is, we no longer create about:blank for the initial browser in process X, only to then immediately remoteness-switch to process Y, throwing everything away and creating another one in process Y, because after bug 1506608 and bug 1509906 we should be determining the right process type for the initial browser. I'm not aware of it ever happening when opening new tabs in extant windows, I think that has worked correctly for quite some time, though if you're aware of issues there I'd love to hear about them. > but there are other odd > corner cases that I can't remember off the top of my head. OK. Let me know if you do remember... > > > I suspect there may be some performance win from not sending those > > > notifications... > > > > We don't send them to JS today, because the document is created before we > > attach the web progress listener, so I don't think there are serious wins to > > be had here (from creating the doc later and then not sending the > > notifications). There may still be other (e.g. observer service) > > notifications that we can avoid sending, I guess, but I've never seen these > > show up in profiles so again, I'm skeptical how much that'd net us. Am I > > misunderstanding your point? > > I've frequently seen them show up in profiles... Typically, the first piece > of code that accidentally creates a contentViewer shows up as taking a huge > chunk of time because of all of the other JS code the content viewer > triggers. Sure, but if the content viewer will be created anyway (ie the docshell isn't about to be destroyed) then not creating about:blank just kicks that proverbial can down the road. We don't actually avoid the cost of creating the content viewer by avoiding the initial about:blank creation. The only thing we avoid is creating the empty doc and any progress/load notifications triggered by that doc - and we only avoid that if the docshell is about to go away or we're about to load another document instead. > > > So, there are essentially two different kinds of about:blank loads: 1) > > > Initial about:blank contentViewer creations that are going to be replaced > > > with a real document. These have document.readyState == "uninitialized", and > > > should generally be ignored. And, 2) real about:blank document loads, either > > > directly loaded, or implicitly loaded for the sake of running things like > > > javascript: URIs. These have document.readyState == "loading", > > > "interactive", or "complete". > > > > I don't think these are distinguishable as you suggest. First, because for > > the initial onStateChange (STATE_START) notification(s?), there isn't yet a > > document to check. Second, because per bug 1505850, I think we create an > > initial document of 'type' 1 in your list for js loads in new tabs, not type > > 2 (at least, we call EnsureContentViewer, so I don't understand how a > > different document would result from the same call). > > The type 1 documents typically get reused to load a subsequent document. > But, regardless, when they're in the uninitialized state, we really want to > avoid touching them as much as possible. They tend to behave in really > unexpected ways. This might imply that we want to get rid of these about:blank docs for other, non-performance reasons? But it doesn't seem like a perf concern, right? > > > Telling them apart can be tricky, but we already have to do it for the sake > > > of extension content scripts, so it's not really much trouble to do the same > > > in other cases. > > > > Can you point me to the relevant code? > > https://searchfox.org/mozilla-central/rev/ > adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/toolkit/components/extensions/ > ExtensionPolicyService.cpp#480-493 OK. This seems to be about the document-created notification, but not about the progress listener stuff... and there aren't many observers for this. I kinda struggled with the webprogress stuff - there are extant checks for this in tabbrowser, but (at a glance) they seem broken, and just happen to work today because we don't get the notifications at all... > > Our code makes lots of assumptions that just accessing the document is OK, > > and/or that currentURI is always non-null. Right now I'm just not convinced > > that all the added complexity of doing that is worth it. > > I know. And I think that we should generally try to reduce those assumptions > as much as possible, because touching uninitialized about:blank content > viewers generally doesn't do what people expect. And the fact that they tend > to get reused to load other documents in arcane and counter-intuitive ways > means that someone assuming that they really are about:blank documents early > on can potentially have serious consequences. Hm. I mean, I'm happy to share my WIP stuff, but having to sprinkle nullchecks for `currentURI` all over the place didn't really feel like making things better... I guess we could stub out currentURI to just lie and return about:blank in this case, but that seems even *more* likely to have nasty side-effects. Are you aware of a different way to avoid this? Is this just a sour apple you think we should get through for sanity's sake? I'm a little surprised in general, because the webby side of this cannot, AFAIU, go away (ie web content expects to be able to create an <iframe> or new window, and then manipulate about:blank in it). If I had to summarize, it sounds like you're saying (a) we should fix this even if there isn't significant perf impact, and (b) there might still be some perf impact. I'm pushing back a bit on (b) above, but if we're convinced enough about (a) it might still be worth pursuing anyway... it's hard to know the exact perf impact here until we fix all the consumers that trip this stuff, because without that we still end up paying whatever perf cost it is... Does that sound right?
Flags: needinfo?(kmaglione+bmo)
One other thought I had here the other day was that (In reply to :Gijs (he/him) from comment #14) > There are 2 ways of fixing this: one is to try to avoid the about:blank > document loading at all. This doesn't seem to be very workable in practice, > as there are quite a number of different ways in which the document can be > created - some through JS, some through platform C++ code - and in some > cases this creation is necessary. As a semi-random example, focusing the > browser in the parent process attempts to focus the content of the browser, > which trips creating a content document in that browser, etc. > > The other is to try to get the tab progress listeners to ignore cases where > they get notified for the initial about:blank load, but that runs the risk > of it ignoring loads that it shouldn't be ignoring (like for js: URIs in a > new tab). So that gets messy. This second option may be necessary anyway if we proceed with bug 1510569, in that if we move responsibility for web progress event listening into our C++ IPC layer, it'll likely attach sooner, and we'll start getting the events that we're meant to be ignoring at the moment anyway...
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1510569

I'll see if we can drop this altogether now that we're moving to fission-compatible actors and a lot of things have changed in the meantime...

Flags: needinfo?(kmaglione+bmo) → needinfo?(gijskruitbosch+bugs)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=779f0cf76f6a0ab9c028984fa0df11301e285053

Summary: Stop touching `content` in browser/base/content/content.js → Stop touching `content` in browser/base/content/tab-content.js

I tried this for a bit, but the first hurdle is that we now have an assert in JSActorManager that checks IsSafeToRunScript. We hit this in some tests, cf. https://treeherder.mozilla.org/#/jobs?repo=try&selectedTaskRun=USYZMgdASTaaDEQ-m8xGNg.0&revision=779f0cf76f6a0ab9c028984fa0df11301e285053

The reason appears to be that we get an onLocationChange notification for the load of about:blank, and the tabbrowser code at https://searchfox.org/mozilla-central/rev/d25eb00ab4e90cc0130cd18f303a04cc2a2f8409/browser/base/content/browser.js#5411 attempts to create an actor at https://searchfox.org/mozilla-central/rev/d25eb00ab4e90cc0130cd18f303a04cc2a2f8409/browser/actors/AboutReaderParent.jsm#169-171 . This fails with the assert.

What I don't understand is why it is not safe to run script - we're already running script! The C++ stack looks like this on my local opt build:

  * frame #0: 0x0000000104b896b9 XUL`mozilla::dom::JSActorManager::GetActor(this=0x0000000125984400, aCx=0x000000011b32f000, aName=0x00007ffeefbfa818, aRv=<unavailable>) at JSActorManager.cpp:18:3 [opt]
    frame #1: 0x0000000103d25b7b XUL`mozilla::dom::WindowGlobalParent_Binding::getActor(cx=0x000000011b32f000, obj=<unavailable>, void_self=0x0000000125984400, args=0x00007ffeefbfa900) at WindowGlobalActorsBinding.cpp:2164:86 [opt]
    frame #2: 0x0000000104028135 XUL`bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(cx=0x000000011b32f000, argc=1, vp=<unavailable>) at BindingUtils.cpp:3229:13 [opt]
    frame #3: 0x00000001063671be XUL`js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [inlined] CallJSNative(cx=0x000000011b32f000, native=(XUL`bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) at BindingUtils.cpp:3203), reason=<unavailable>, args=0x00007ffeefbfaac8)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) at Interpreter.cpp:509:13 [opt]
    frame #4: 0x0000000106367171 XUL`js::InternalCallOrConstruct(cx=0x000000011b32f000, args=0x00007ffeefbfaac8, construct=<unavailable>, reason=<unavailable>) at Interpreter.cpp:601 [opt]
    frame #5: 0x00000001063607e9 XUL`Interpret(JSContext*, js::RunState&) [inlined] js::CallFromStack(cx=0x000000011b32f000, args=<unavailable>) at Interpreter.cpp:670:10 [opt]
    frame #6: 0x00000001063607df XUL`Interpret(cx=0x000000011b32f000, state=<unavailable>) at Interpreter.cpp:3338 [opt]

plus some more JS/jit frames, so not particularly illuminating. :-(

Nika or Kris, can you shed some light on this?

Flags: needinfo?(nika)
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(gijskruitbosch+bugs)

I chatted with you a bit about this on Matrix. This isn't super high priority, and fully preventing creating the initial about:blank document would be very difficult, so I'm inclined to put this on the back burner for now.

Flags: needinfo?(nika)

(In reply to :Gijs (he/him) from comment #22)

What I don't understand is why it is not safe to run script - we're already running script! The C++ stack looks like this on my local opt build:

Unfortunately, it's still possible to run scripts at times when it isn't safe to run scripts, and in the past we've been fairly lenient about it for chrome scripts, despite how footgunny it is. It's fairly easy for those scripts to trigger code that really isn't safe to run when we have a script blocker. This probably isn't one of those cases, but those assertions are mainly in place to prevent native code from accidentally instantiating an actor when it isn't safe to run scripts.

We're winding up doing that here:

#45 0x000015554f0eb7fe in nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal*, nsIPrincipal*, nsIContentSecurityPolicy*, nsIURI*, mozilla::Maybe<nsILoadInfo::CrossOriginEmbedderPolicy> const&, bool, bool, mozilla::dom::WindowGlobalChild*) (this=this@entry=0x155554d2ac00, aPrincipal=aPrincipal@entry=
0x0, aPartitionedPrincipal=aPartitionedPrincipal@entry=0x0, aCSP=<optimized out>, aBaseURI=<optimized out>, aCOEP=..., aTryToSaveOldPresentation=<optimized out>, aCheckPermitUnload=<optimized out>, aActor=0x0) at /home/kris/code/mozilla-central/docshell/base/nsDocShell.cpp:6758
#46 0x000015554f0d6d58 in nsDocShell::EnsureContentViewer() (this=0x155554d2ac00) at /home/kris/code/mozilla-central/docshell/base/nsDocShell.cpp:6575
#47 0x000015554f0e0148 in non-virtual thunk to nsDocShell::GetDocument() () at /home/kris/code/mozilla-central/docshell/base/nsDocShell.cpp:3179
#48 0x000015554e11c90c in mozilla::PresShell::GetPrimaryContentDocument() (this=<optimized out>) at /home/kris/code/mozilla-central/layout/base/PresShell.cpp:8005
#49 0x000015554e11be24 in mozilla::PresShell::Paint(nsView*, nsRegion const&, mozilla::PaintFlags) (this=0x155540907000, aViewToPaint=0x155540ed5b80, aDirtyRegion=..., aFlags=mozilla::PaintFlags::PaintLayers) at /home/kris/code/mozilla-central/layout/base/PresShell.cpp:6181
#50 0x000015554dedb73b in nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) (this=<optimized out>, this@entry=0x155540fa74c0, aWidget=aWidget@entry=0x15554164f400) at /home/kris/code/mozilla-central/view/nsViewManager.cpp:460
#51 0x000015554dedb3ea in nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) (this=this@entry=0x155540fa74c0, aView=<optimized out>, aFlushDirtyRegion=false) at /home/kris/code/mozilla-central/view/nsViewManager.cpp:395
#52 0x000015554dedc76d in nsViewManager::ProcessPendingUpdates() (this=0x155540fa74c0) at /home/kris/code/mozilla-central/view/nsViewManager.cpp:1018
#53 0x000015554e0ed635 in nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) (this=0x155540e40800, aId=aId@entry=..., aNowTime=...) at /home/kris/code/mozilla-central/layout/base/nsRefreshDriver.cpp:2369
#54 0x000015554e0f23d0 in mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver*, mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) (driver=0x0, aId=..., now=...) at /home/kris/code/mozilla-central/layout/base/nsRefreshDriver.cpp:374
#55 mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) (this=this@entry=0x155540fee940, aId=aId@entry=..., aNow=aNow@entry=..., aDrivers=nsTArray<RefPtr<nsRefreshDriver> > & = {...})
at /home/kris/code/mozilla-central/layout/base/nsRefreshDriver.cpp:353
#56 0x000015554e0f21fd in mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) (this=this@entry=0x155540fee940, aId=..., now=now@entry=...) at /home/kris/code/mozilla-central/layout/base/nsRefreshDriver.cpp:368
#57 0x000015554e0f1d6a in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) (this=0x155540fee940, aId=..., aTimeStamp=...) at /home/kris/code/mozilla-central/layout/base/nsRefreshDriver.cpp:829
#58 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) (this=this@entry=0x155540dbe160, aId=..., aId@entry=..., aVsyncTimestamp=aVsyncTimestamp@entry=...) at /home/kris/code/mozilla-central/layout/base/nsRefreshDriver.cpp:747
#59 0x000015554e0f1ab9 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyParentProcessVsync() (this=0x155540dbe160) at /home/kris/code/mozilla-central/layout/base/nsRefreshDriver.cpp:649
#60 0x000015554e0f10b0 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() (this=0x155542ba46d0) at /home/kris/code/mozilla-central/layout/base/nsRefreshDriver.cpp:539

We definitely don't want to be creating new content viewers in the middle of layout/painting. There are lots of ways that could trigger unsafe code... Which means I think we want that code to do something like GetExtantDocument instead.

Flags: needinfo?(kmaglione+bmo)

We nowadays always eagerly create the initial about:blank document in the BrowserChild case in order to eagerly attach it to the provided WindowGlobalChild which was passed when creating the PBrowser actor. Because the vast majority of toplevel content we end up creating is remote these days, I'm skeptical that this still has a significant performance impact, though it is certainly gross.

I just filed bug 1708245 to track getting rid of tab-content.js entirely, as it's getting fairly small at this point. Once the only remaining action which that frame script performs becomes initializing the initial about:blank document, I think we may want to move the eager-about:blank initialization for toplevel content BrowsingContexts logic into nsFrameLoader or similar, rather than keeping it in frontend JS as a hackaround, as that will let us avoid loading the framescript completely.

Assignee: nobody → nika
Status: NEW → ASSIGNED

https://hg.mozilla.org/mozilla-central/rev/7b3a831a0f6b

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox90: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: