Closed
Bug 1356334
Opened 8 years ago
Closed 7 years ago
Add UI for flagging long running Web Extension scripts and provide the option to stop them
Categories
(WebExtensions :: Frontend, enhancement, P1)
WebExtensions
Frontend
Tracking
(Performance Impact:high, firefox57 fixed)
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: kmag)
References
Details
(Whiteboard: , triaged)
Attachments
(7 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
bholley
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kmag
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mixedpuppy
:
review+
|
Details |
30.69 KB,
image/png
|
Details |
See bug 1345633 comment 12.
It would be nice to customize our long running script on web pages UI to cover Web Extensions, something along like:
[Addon Foo] is making Firefox run slowly, what would you like to do?
[ Wait ] [ Stop ]
We can potentially use a shorter timeout for add-on content script sandboxes to avoid disaster situations like bug 1345633.
Assignee | ||
Comment 1•8 years ago
|
||
We should probably also include the URL or title of the page the content script is running on.
And, in the case of content scripts, unlike page scripts, we can kill the content script entirely rather than just the current stack, so we should do that either as an option or by default.
Assignee | ||
Comment 2•8 years ago
|
||
Most of the code to handle slow scripts is here:
http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/js/xpconnect/src/XPCJSContext.cpp#1313
Ideally, I'd like to separately record the time spent in chrome/content/extension compartments since the last checkpoint, but the current code just checks the context at the top of the stack when the interrupt happens. Since I think we're probably better off having something soon than something perfect, I'll just stick to that logic for now, and hopefully we can do something better down the road.
Assignee: nobody → kmaglione+bmo
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•8 years ago
|
||
Ideally this would have tests, but there don't appear to be any existing tests for the slow script checker, and I don't have any particularly good ideas about how to implement them from scratch.
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8858664 [details]
Bug 1356334: Part 1 - Add helpers for safely casting principals without COM overhead.
https://reviewboard.mozilla.org/r/130640/#review133302
Attachment #8858664 -
Flags: review?(bobbyholley) → review+
Comment 12•8 years ago
|
||
In bug 1309946 we removed the "this addon might be making Firefox run slowly" for a few reasons, it was inaccurate and it didn't seem that effective. I'm concerned about any user facing warning from WebExtensions for similar reasons.
At the least, can we get this on a Nightly only until we've got some real world feedback?
Does this path have any telemetry showing how often this is shown?
Need info'ing Kev because I'm sure he'd have some opinion on this one.
Flags: needinfo?(kev)
Comment 14•8 years ago
|
||
Spoke to Kris and he pointed out that this warning already happens, this bug just changes it so it shows the add-on in the script instead of something intelligible to the end user. Based on that not worried about this any more and removing the need info. Sorry about the noise.
Flags: needinfo?(kev)
Flags: needinfo?(amckay)
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8858670 [details]
Bug 1356334: Part 7 - Destroy content script sandboxes after slow script termination.
https://reviewboard.mozilla.org/r/130652/#review133458
Attachment #8858670 -
Flags: review?(mixedpuppy) → review+
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8858665 [details]
Bug 1356334: Part 3 - Enforce a stricter slow script timeout for extension content scripts.
https://reviewboard.mozilla.org/r/130642/#review134518
I'm pretty sure this will break bug 1329731 (or something like it). As we discussed, we need to use a different concept of "scriptability" for killing scripts due to timeouts than we use for never allowing JS at all.
::: js/xpconnect/src/XPCJSContext.cpp:352
(Diff revision 1)
>
> MOZ_CRASH("Chain parser loop does not terminate");
> }
>
> +static inline bool
> +IsExtensionPrincipal(nsIPrincipal* principal, nsAString& addonId)
Please call this IsWebExtensionPrincipal. It's a little weird that GetAddonId below only returns values for WebExtensions, so I'd like to make it clear here.
::: js/xpconnect/src/XPCJSContext.cpp:359
(Diff revision 1)
> + return (NS_SUCCEEDED(principal->GetAddonId(addonId)) &&
> + !addonId.IsEmpty());
> +}
> +
> +static bool
> +IsExtensionContentScript(BasePrincipal* principal, nsAString& addonId)
This should be called IsWebExtensionContentScript.
::: js/xpconnect/src/XPCJSContext.cpp:390
(Diff revision 1)
> - // ExpandedPrincipal gets a free pass.
> - nsCOMPtr<nsIExpandedPrincipal> ep = do_QueryInterface(aPrincipal);
> - if (ep)
> + auto principal = BasePrincipal::Cast(aPrincipal);
> +
> + // ExpandedPrincipal gets a free pass unless it's an extension content script.
> + nsString addonId;
> + if (principal->Is<ExpandedPrincipal>()) {
> + return !IsExtensionContentScript(principal, addonId);
I'm really confused. So when JS is disabled, then WebExtension background pages are still allowed to run. WebExtension content scripts are not. And other content scripts (SDK, etc.) are allowed to run. What's the justification?
Attachment #8858665 -
Flags: review?(wmccloskey) → review-
Comment 17•8 years ago
|
||
mozreview-review |
Comment on attachment 8858666 [details]
Bug 1356334: Part 3 - Enforce a stricter slow script timeout for extension content scripts.
https://reviewboard.mozilla.org/r/130644/#review134526
::: js/xpconnect/src/XPCJSContext.cpp:1226
(Diff revision 1)
> if (chromeTime <= 0)
> chromeTime = INT32_MAX;
> - mWatchdog->SetMinScriptRunTimeSeconds(std::min(contentTime, chromeTime));
> + int32_t extTime = Preferences::GetInt(PREF_MAX_SCRIPT_RUN_TIME_EXT_CONTENT, 5);
> + if (extTime <= 0)
> + extTime = INT32_MAX;
> + mWatchdog->SetMinScriptRunTimeSeconds(std::min(std::min(contentTime, chromeTime), extTime));
I think you can now do:
std::min({a, b, c})
::: js/xpconnect/src/XPCJSContext.cpp:1387
(Diff revision 1)
> + auto principal = BasePrincipal::Cast(nsContentUtils::SubjectPrincipal(cx));
> + bool chrome = principal->Is<SystemPrincipal>();
> + if (chrome) {
> + prefName = PREF_MAX_SCRIPT_RUN_TIME_CHROME;
> + limit = Preferences::GetInt(prefName, 20);
> + } else if (IsExtensionContentScript(principal, addonId)) {
The function you defined in part 2 only worked for WebExtensions. It would be nice if we could instead use the add-on ID from the current compartment. That should work more generally.
Attachment #8858666 -
Flags: review?(wmccloskey) → review+
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8858667 [details]
Bug 1356334: Part 4 - Add a helper to synchronously get an extension's name from its ID.
https://reviewboard.mozilla.org/r/130646/#review134538
It's unfortunate that this only works for WebExtensions, but I looked at the add-on manager and it seems like it would be horrible to make it work more generally :-(. Now I'm not sure if we should make it work for normal add-ons or not.
::: toolkit/components/extensions/ExtensionManagement.jsm:86
(Diff revision 1)
> initialized: false,
>
> // Map[uuid -> extension].
> // extension can be an Extension (parent process) or BrowserExtensionContent (child process).
> uuidMap: new Map(),
> + idMap: new Map(),
Please make a similar comment to the one above about the domain and range of this map.
Attachment #8858667 -
Flags: review?(wmccloskey) → review+
Comment 19•8 years ago
|
||
mozreview-review |
Comment on attachment 8858668 [details]
Bug 1356334: Part 5 - Add add-on name to slow script messages.
https://reviewboard.mozilla.org/r/130648/#review134578
::: dom/base/nsGlobalWindow.cpp:11662
(Diff revision 1)
> + failed = failed || NS_FAILED(rv) || result.IsEmpty();
> + return Move(result);
> + };
> +
> + bool isAddonScript = !aAddonId.IsEmpty();
> + bool showDebugButton = debugCallback && !isAddonScript;
Does the debug button not work for add-ons?
Attachment #8858668 -
Flags: review?(wmccloskey) → review+
Comment 20•8 years ago
|
||
mozreview-review |
Comment on attachment 8858669 [details]
Bug 1356334: Part 6 - Allow completely terminating a slow content script sandbox.
https://reviewboard.mozilla.org/r/130650/#review134598
::: js/xpconnect/src/XPCJSContext.cpp:1466
(Diff revision 1)
> if (response == nsGlobalWindow::KillSlowScript) {
> if (Preferences::GetBool("dom.global_stop_script", true))
> xpc::Scriptability::Get(global).Block();
> return false;
> }
> + if (response == nsGlobalWindow::KillScriptGlobal) {
I'm unclear on exactly how this differs from KillSlowScript when the dom.global_stop_script pref is set. Can you explain?
::: js/xpconnect/src/XPCJSContext.cpp:1477
(Diff revision 1)
> + // Notify the extensions framework that the sandbox should be killed.
> + nsIXPConnect* xpc = nsContentUtils::XPConnect();
> + JS::RootedObject wrapper(cx, JS_NewPlainObject(cx));
> + nsCOMPtr<nsISupports> supports;
> +
> + if (!wrapper ||
Please comment what this is meant to do. I'd feel better if Blake looked at this part. Maybe there's a better way to do it that I'm not aware of.
Attachment #8858669 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 21•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8858667 [details]
Bug 1356334: Part 4 - Add a helper to synchronously get an extension's name from its ID.
https://reviewboard.mozilla.org/r/130646/#review134538
Yeah, I was thinking about trying to make it work for normal add-ons as a follow-up. The problem is that the add-on manager doesn't know the name of the add-on until the database is loaded, and we don't normally load it at startup for performance reasons (although telemetry currently forces it to be loaded pretty early...)
I was considering adding a private synchronous API to get the name of an add-on, and synchronously load the DB if necessary, but I'm a bit worried that it might be abused. Anyway, I'll file a follow-up for that. I just didn't want this to block on it.
Assignee | ||
Comment 22•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8858666 [details]
Bug 1356334: Part 3 - Enforce a stricter slow script timeout for extension content scripts.
https://reviewboard.mozilla.org/r/130644/#review134526
> I think you can now do:
> std::min({a, b, c})
Oh, nice.
> The function you defined in part 2 only worked for WebExtensions. It would be nice if we could instead use the add-on ID from the current compartment. That should work more generally.
Well, we only want to apply this to content script compartments, which means we'd have to either check if the global is a sandbox, or check the isWebExtensionContentScript compartment flag. Checking the global seems a bit flaky, and checking the compartment flag would still only give us WebExtension sandboxes anyway.
Assignee | ||
Comment 23•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8858665 [details]
Bug 1356334: Part 3 - Enforce a stricter slow script timeout for extension content scripts.
https://reviewboard.mozilla.org/r/130642/#review134518
> I'm really confused. So when JS is disabled, then WebExtension background pages are still allowed to run. WebExtension content scripts are not. And other content scripts (SDK, etc.) are allowed to run. What's the justification?
I was mainly just trying to limit the changes to what was necessary for this bug. I think it probably makes sense to remove the excemption for expanded principals and WebExtension principals in general, though, so I'm happy to do that if that's what you'd prefer.
Assignee | ||
Comment 24•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8858668 [details]
Bug 1356334: Part 5 - Add add-on name to slow script messages.
https://reviewboard.mozilla.org/r/130648/#review134578
> Does the debug button not work for add-ons?
As far as I can tell, it doesn't. But I was mainly just trying to avoid having to deal with all of the extra message possibilities in the debug+addonId case.
Assignee | ||
Comment 25•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8858669 [details]
Bug 1356334: Part 6 - Allow completely terminating a slow content script sandbox.
https://reviewboard.mozilla.org/r/130650/#review134598
> I'm unclear on exactly how this differs from KillSlowScript when the dom.global_stop_script pref is set. Can you explain?
In the case of a content script sandbox (which is the only case this is returned for), we actually completely and cleanly teardown the global, rather than just blocking scripts. But I realized after I pushed this that we probably shouldn't block scriptability in the KillSlowScript case for content scripts.
> Please comment what this is meant to do. I'd feel better if Blake looked at this part. Maybe there's a better way to do it that I'm not aware of.
I couldn't think of or find one. The only other places I could find where we were doing anything like this (e.g., [1]), we were doing it more or less the same way, so I decided it was good enough.
[1]: http://searchfox.org/mozilla-central/rev/d8ac097a1de2544f0e51d186bc850662c5b7430a/dom/console/Console.cpp#1089
Reporter | ||
Comment 26•8 years ago
|
||
Thanks a lot Kris for the patches!
Hi Markus, I asked bwinton for some UX opinion about how to message this in the UI since he worked on WebExtensions UX and he recommended to get your help on this. Part 5 of this patchset adds a message specific to WebExtensions here when the injected content script is slowing down the page, do you mind taking a look please and provide some feedback? Thanks!
Flags: needinfo?(mjaritz)
Comment 27•8 years ago
|
||
Hi Ehsan, Kris, can you please share the mentioned UI with me as a screenshot, or a screen recording if interactivity is important.
Or as a test-build and info on how to trigger the UI if you think I might need a really close look. Thank you!
Flags: needinfo?(ehsan)
Assignee | ||
Comment 28•8 years ago
|
||
Here's the e10s version:
https://people.mozilla.com/~kmaglione/images/b5ceac730f4e2c47.png
And the non-e10s version:
https://people.mozilla.com/~kmaglione/images/594872b50356a186.png
I can get you a link to a try build to test on, if necessary, but I don't think they'll tell you much more than the screenshots.
Flags: needinfo?(ehsan)
Assignee | ||
Comment 29•8 years ago
|
||
Also note that none of this UI is new. The only changes are the messaging for slow scripts that belong to add-ons, and the option to kill the content script.
Comment 30•8 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #28)
> Here's the e10s version:
> https://people.mozilla.com/~kmaglione/images/b5ceac730f4e2c47.png
>
> And the non-e10s version:
> https://people.mozilla.com/~kmaglione/images/594872b50356a186.png
Looks good to me. But it would be great to have Michelle check the copy on it as that seams to be the crucial aspect.
Looks like a typo in the non-e10s version. responding instead of responsing
> I can get you a link to a try build to test on, if necessary, but I don't
> think they'll tell you much more than the screenshots.
Shouldn't be necessary. Thanks.
Flags: needinfo?(mjaritz) → needinfo?(mheubusch)
Updated•8 years ago
|
Priority: -- → P1
Whiteboard: [qf:p1] → [qf:p1], triaged
Comment 31•8 years ago
|
||
(In reply to Markus Jaritz [:designakt prev :maritz] (UX) from comment #30)
> (In reply to Kris Maglione [:kmag] from comment #28)
> > Here's the e10s version:
> > https://people.mozilla.com/~kmaglione/images/b5ceac730f4e2c47.png
> >
> > And the non-e10s version:
> > https://people.mozilla.com/~kmaglione/images/594872b50356a186.png
>
> Looks good to me. But it would be great to have Michelle check the copy on
> it as that seams to be the crucial aspect.
>
> Looks like a typo in the non-e10s version. responding instead of responsing
>
>
> > I can get you a link to a try build to test on, if necessary, but I don't
> > think they'll tell you much more than the screenshots.
> Shouldn't be necessary. Thanks.
Are these two versions (e10s and non-e10s) meant to communicate the same thing? If so, can I write one set of strings and button copy for both cases? Why is one a notification bar and the other a pop-up window? Also, can someone explain the difference between disabling the add-on and stopping it or is it stopping the script only not the add-on? Also, what happens if I choose wait and there is still a problem? will I get another opportunity to choose stop?
I'd like to recommend something like the following but want to make sure I've covered the cases:
A script in <extension name> is causing <Firefox> to slow down.
You can disable this extension, stop the script, or wait. Learn more (links to SUMO)
[Disable Extension][Stop Script] [Wait]
Flags: needinfo?(mjaritz)
Flags: needinfo?(mheubusch)
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 32•8 years ago
|
||
(In reply to mheubusch from comment #31)
> Are these two versions (e10s and non-e10s) meant to communicate the same
> thing? If so, can I write one set of strings and button copy for both cases?
> Why is one a notification bar and the other a pop-up window?
They're more or less mean to communicate the same thing, but there's a
fundamental difference between the e10s and non-e10s cases: In e10s, the
script only blocks the content process. The main browser UI is still
responsive, but the content pages running in the same process largely aren't.
In the non-e10s case, the entire browser UI is frozen while the script is
executing, so we need to open a modal dialog and deal with it immediately.
> Also, can someone explain the difference between disabling the add-on and
> stopping it or is it stopping the script only not the add-on?
In the first case, we stop the add-on from running on the page at all, until
the next time it loads. So if it has timers, for instance, that repeatedly
cause problems, those won't bet a chance to run anymore.
In the second case, we just interrupt the current operation, but the add-on
will still be able to interact with the page.
> Also, what happens if I choose wait and there is still a problem? will I get
> another opportunity to choose stop?
Yes, after the slow script delay elapses again. Currently that's 5 seconds for
add-on content scripts.
Flags: needinfo?(kmaglione+bmo)
Comment 33•8 years ago
|
||
(In reply to mheubusch from comment #31)
> Are these two versions (e10s and non-e10s) meant to communicate the same
> thing? If so, can I write one set of strings and button copy for both cases?
Without e10s the whole browser will become unresponsive as all UI is part of the same process that is slowed down. Therefor separate dialog. And because it is a more severe hang than what people will experience with e10s.
So in the non-e10s case the browser will be completely unresponsive prior to the message, where as with e10s UI will still be clickable while content got slowed down. (Therefor I think a difference in wording makes sense: unresponsive - slow )
> Why is one a notification bar and the other a pop-up window?
Pop-up because the whole rest of the browser is unresponsive. And notification bar, because it is usual warning UI.
> Also, can someone explain the difference between disabling the add-on and
> stopping it or is it stopping the script only not the add-on?
I read "it" as referring to the script. (but I see how that can be misleading after the first button.)
> Also, what happens if I choose wait and there is still a problem? will I get
> another opportunity to choose stop?
I think I recall it re-surfacing after a while. (I see this as the expert option for people that know what they are doing...)
> I'd like to recommend something like the following but want to make sure
> I've covered the cases:
>
> A script in <extension name> is causing <Firefox> to slow down.
>
> You can disable this extension, stop the script, or wait. Learn more (links
> to SUMO)
> [Disable Extension][Stop Script] [Wait]
As the non-e10s hang will be much more severe, I think warrants some more explanation. (and if we reduce the copy of it, we should consider reducing the copy on scripts from web pages too. (currently the copy is very similar to that one)
Comment 35•8 years ago
|
||
(In reply to Markus Jaritz [:designakt prev :maritz] (UX) from comment #33)
> (In reply to mheubusch from comment #31)
> > Are these two versions (e10s and non-e10s) meant to communicate the same
> > thing? If so, can I write one set of strings and button copy for both cases?
>
> Without e10s the whole browser will become unresponsive as all UI is part of
> the same process that is slowed down. Therefor separate dialog. And because
> it is a more severe hang than what people will experience with e10s.
> So in the non-e10s case the browser will be completely unresponsive prior to
> the message, where as with e10s UI will still be clickable while content got
> slowed down. (Therefor I think a difference in wording makes sense:
> unresponsive - slow )
>
> > Why is one a notification bar and the other a pop-up window?
> Pop-up because the whole rest of the browser is unresponsive. And
> notification bar, because it is usual warning UI.
>
> > Also, can someone explain the difference between disabling the add-on and
> > stopping it or is it stopping the script only not the add-on?
>
> I read "it" as referring to the script. (but I see how that can be
> misleading after the first button.)
>
> > Also, what happens if I choose wait and there is still a problem? will I get
> > another opportunity to choose stop?
>
> I think I recall it re-surfacing after a while. (I see this as the expert
> option for people that know what they are doing...)
>
> > I'd like to recommend something like the following but want to make sure
> > I've covered the cases:
> >
> > A script in <extension name> is causing <Firefox> to slow down.
> >
> > You can disable this extension, stop the script, or wait. Learn more (links
> > to SUMO)
> > [Disable Extension][Stop Script] [Wait]
>
> As the non-e10s hang will be much more severe, I think warrants some more
> explanation. (and if we reduce the copy of it, we should consider reducing
> the copy on scripts from web pages too. (currently the copy is very similar
> to that one)
So, for e10s I recommend:
A script in the extension <extension name> is causing <Firefox> to slow down. <a href="https://support.mozilla.org/en-US/kb/warning-unresponsive-script?cache=no#w_other-causes" Learn more</a>
[Temporarily Disable Extension][Stop Script] [Wait]
For non-e10s I still have a few questions:
1. Is it possible to also disable the extension, or are the only options Stop Script or Continue?
2. Does "Continue" mean wait? Does a user actually have to press the button or will the script keep running in on the page until you select the Stop Script button?
3. I don't understand the checkbox: Prevent the add-on script from running . . . - Does this mean "temporarily disable the whole add on (like in e10s) or something else?
4. Why is the name of the script displayed? Can a user click on this or report it to someone?
Thanks in advance for your help.
Comment 36•8 years ago
|
||
Hi Markus and Kris, Just NI'ing you to make sure you saw my additional questions.
Flags: needinfo?(mjaritz)
Flags: needinfo?(kmaglione+bmo)
Comment 37•8 years ago
|
||
(In reply to mheubusch from comment #35)
>
> So, for e10s I recommend:
> A script in the extension <extension name> is causing <Firefox> to slow
> down. <a
> href="https://support.mozilla.org/en-US/kb/warning-unresponsive-
> script?cache=no#w_other-causes" Learn more</a>
> [Temporarily Disable Extension][Stop Script] [Wait]
Great. Thanks.
> For non-e10s I still have a few questions:
Non-e10s looks rather complicated, but with us expanding e10s more and more, it will soon be gone for good.
And as it is basically the same thing that people have seen for slow scripts from websites for years, I think it is something people know (even if it is not the nicest of dialogues).
Therefor I did not put a lot of focus on it and would like to keep the effort for changes low.
(If you think we should re-word, we should also make sure to update the non-e10s slow script dialog for websites.)
> 1. Is it possible to also disable the extension, or are the only options
> Stop Script or Continue?
The checkbox to block the script blocks a good part of the extension. If it is the extensions main script, it would be effectively be blocking the extension, but if it is only one of many scripts in the extension, I assume not.
> 2. Does "Continue" mean wait?
Yes.
> Does a user actually have to press the button
> or will the script keep running in on the page until you select the Stop
> Script button?
I am not sure I know what you mean.
> 3. I don't understand the checkbox: Prevent the add-on script from running .
> . . - Does this mean "temporarily disable the whole add on (like in e10s)
> or something else?
This probably depends on what script is running slow.
> 4. Why is the name of the script displayed? Can a user click on this or
> report it to someone?
It can be helpful for the developer to know what caused the slow-down, but a user can not click on it.
(this would be a relic from a different approach to error messages as we do them now.)
But I accepted it as it follows the pattern of the slow script warnings websites gave for a long time, and as it will only be seen by people that have no e10s yet.
(I should have expressed that earlier, when asking for your feedback Michelle, to give some sense of scope.)
> Thanks in advance for your help.
Flags: needinfo?(mjaritz)
Comment 38•8 years ago
|
||
As Andy asked me in SF about this, I want to summarize what we should do here:
We should follow Michelle's copy for e10s:
(In reply to mheubusch from comment #35)
> So, for e10s I recommend:
> A script in the extension <extension name> is causing <Firefox> to slow down. <a href="https://support.mozilla.org/en-US/kb/warning-unresponsive-script?cache=no#w_other-causes" Learn more</a>
[Temporarily Disable Extension][Stop Script] [Wait]
and for non-e10s we should keep what we have, as non-e10s is not a priority and transitioning users to e10s should be complete by 57.
Assignee | ||
Comment 39•7 years ago
|
||
Sorry I haven't had much chance to get back to this the past few months. Given bugs like bug 1369274, though, it's back at the top of my priority list.
I'm dropping part 2, since nuking the sandbox should be enough in most cases, and it looks like Scriptability::Block() should work even for principals that are immune to script policies.
I updated the slow script info bar with the suggested text and documentation link (screenshot attached).
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8858666 -
Attachment is obsolete: true
Assignee | ||
Comment 46•7 years ago
|
||
Comment on attachment 8858667 [details]
Bug 1356334: Part 4 - Add a helper to synchronously get an extension's name from its ID.
This part had to change pretty significantly after the rebase. Needs a new review.
Attachment #8858667 -
Flags: review+ → review?(wmccloskey)
Assignee | ||
Updated•7 years ago
|
Attachment #8858665 -
Flags: review?(wmccloskey) → review+
Attachment #8858667 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 47•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c880eca810ac03509cc3102bd421b5f386e82cb
Bug 1356334: Part 1 - Add helpers for safely casting principals without COM overhead. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/805c568069301ae91ead5780cdc118af73907229
Bug 1356334: Part 3 - Enforce a stricter slow script timeout for extension content scripts. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a663ffd1446f9f70b220866855a0ecb3503761
Bug 1356334: Part 4 - Add a helper to synchronously get an extension's name from its ID. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/b83aea215a82d44bec7443b69e60feef32f5fb2c
Bug 1356334: Part 5 - Add add-on name to slow script messages. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/d832803270ac831fd760356f36e16ef2a2d6d45b
Bug 1356334: Part 6 - Allow completely terminating a slow content script sandbox. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5bcd2b2dc695397c97f0c1391b66c6f8251939
Bug 1356334: Part 7 - Destroy content script sandboxes after slow script termination. r=mixedpuppy
Assignee | ||
Comment 48•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/16b49fd1c38a9d978b7b32201687d4499ce50586
Bug 1356334: Follow-up: Fix debug build bustage.
Assignee | ||
Comment 49•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c677ebfdda09a2aad05a30427330eefec43b128
Bug 1356334: Follow-up: Fix quote styles in locale strings.
Comment 50•7 years ago
|
||
Backed out for making test_ext_contentscript_async_loading.html nearly permafail on Android debug. Please be sure to fold those follow-up bustage fixes into the revised patches too while you're at it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/399db8547abb4549b05ffd419fab29c8cbd6c2ae
https://treeherder.mozilla.org/logviewer.html#?job_id=124157172&repo=mozilla-inbound
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 51•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dde6ae5176d04e63905eb5c7c47178371d94260
Bug 1356334: Part 1 - Add helpers for safely casting principals without COM overhead. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b9185a04fe62a75a5a5b97947cb47f81ba2bfd3
Bug 1356334: Part 3 - Enforce a stricter slow script timeout for extension content scripts. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ebfb0df0958366c63048e2b943ce4f3827fbcf0
Bug 1356334: Part 4 - Add a helper to synchronously get an extension's name from its ID. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/6aa43c3fc0c5fa4cf17714bbe8617990503ab1ef
Bug 1356334: Part 5 - Add add-on name to slow script messages. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f57bca8977c2106570607f76c69a68ffae8f52b
Bug 1356334: Part 6 - Allow completely terminating a slow content script sandbox. r=billm
Comment 52•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1dde6ae5176d
https://hg.mozilla.org/mozilla-central/rev/4b9185a04fe6
https://hg.mozilla.org/mozilla-central/rev/7ebfb0df0958
https://hg.mozilla.org/mozilla-central/rev/6aa43c3fc0c5
https://hg.mozilla.org/mozilla-central/rev/6f57bca8977c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 53•7 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #47)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d832803270ac831fd760356f36e16ef2a2d6d45b
> +KillAddonScriptGlobalMessage=Prevent the add-on script from running on this page until it next reloads.
If this is a checkbox with a user-selectable option, it should not contain a period.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(kmaglione+bmo)
Comment 54•7 years ago
|
||
Hello Kris,
Can you please attach some test webextensions which cause the long running script warnings? Also if you can provide some STRs, it will be more clear for me how to exactly test this issue.
Thanks,
Victor
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(kmaglione+bmo) → qe-verify-
Updated•7 years ago
|
Product: Toolkit → WebExtensions
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1], triaged → , triaged
You need to log in
before you can comment on or make changes to this bug.
Description
•