Closed Bug 820438 Opened 7 years ago Closed 7 years ago

When in content process nsJSEnvironment fails to update its pref

Categories

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

defect

Tracking

()

RESOLVED FIXED
B2G C4 (2jan on)
blocking-b2g -
blocking-basecamp -
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: ochameau, Assigned: gsvelto)

References

Details

Attachments

(2 files, 27 obsolete files)

3.44 KB, patch
jst
: review+
Details | Diff | Splinter Review
3.44 KB, patch
jst
: review+
Details | Diff | Splinter Review
/dom/base/nsJSEnvironment.cpp get and set dom.max_script_run_time preferences. But as it is executed in content processes, call to Preferences::SetInt fails.
So that when we click on "Don't ask me again", the preferences isn't updated here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#892
and the prompt asking to stop or continue the slow script keeps opening.

I haven't found any preference API working in content process except nsIContentPrefService which contains a whitelist of preferences that can be set globally.
Can we use this service from the chrome process as well ?
Should we use this service and add dom.max_script_run_time to the whitelist ?

TBH, I don't know how to fix that issue...

In the meantime, we will most likely disable this modal in B2G in bug 817230.
Blocks: 817230
The issue is that we don't trust the code running in the content processes.  So it would be a security hole if we allowed content processes to set any pref arbitrarily.

Last I checked our security model said it was fine for a malicious child process to DoS the phone, so allowing it to set this pref sounds fine to me.

(The right thing would be to set this pref on a per-process basis, but it doesn't sound like we can do that.)
Blocks: 821980
Blocks a blocker - assigning to Alex, since Ben is out (please reassign as necessary).
Assignee: nobody → poirot.alex
blocking-basecamp: --- → +
Priority: -- → P1
Target Milestone: --- → B2G C3 (12dec-1jan)
I tried to fix that issue with this patch without success.
I haven't had time to dig why it still fails, but if someone see an obvious issue in this patch... any help would be much appreciated!
Comment on attachment 694484 [details] [diff] [review]
Bug 820438 - When in content process nsJSEnvironment fails to update its pref

Presumably the code that checks dom.max_chrome_script_run_time is still looking at the global pref instead of the per-site one.
Attachment #694484 - Attachment is obsolete: true
(In reply to Josh Matthews [:jdm] from comment #5)
> Presumably the code that checks dom.max_chrome_script_run_time is still
> looking at the global pref instead of the per-site one.

Oh I was convinced that nsIContentPrefService was forwarding global prefs to parent process and save the pref in the regular pref database...

Here is a new patch that add observers against the per-site database,
but for some reason it still fails.
`nsContentPrefObserver::OnContentPrefSet` is called with correct values when we call `contentPrefService->SetPref()`, but it doesn't seem to update the right `sMaxScriptRunTime` static value.
Can it be some thread/process issues where nsIContentPrefService calls only one observer in a distinct process?

I don't feel very productive in modifying this c++ code,
and as I'm off for a week, feel free to pick it up.
Assignee: poirot.alex → nobody
I have looked at a somewhat similar issue in the past and while dogfooding I was quite annoyed that heavy scripts would basically render the browser unusable so I'm taking it.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
I've looked into the issue and the problem is similar to what I had encountered in the past when dealing with memory-pressure events notifications.

When a child process invokes contentPrefService->SetPref() the call is delivered via IPC to the parent which sets the preference accordingly and notifies the observers. This last step affects only the observers that had registered themselves in the parent process, not the ones registered in a child because the observer service does not deliver notifications via IPC. The nsContentPrefObserver::OnContentPrefSet() method is thus called only in the main b2g process and not in the browser (which is a child process).

I will update the patch with the necessary changes to forward the notifications to the child processes via IPC.
(In reply to Gabriele Svelto [:gsvelto] from comment #9)
> When a child process invokes contentPrefService->SetPref() the call is
> delivered via IPC to the parent which sets the preference accordingly

Oh ... that's not supposed to work :(.  Child processes shouldn't be able to set global prefs.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #10)
> (In reply to Gabriele Svelto [:gsvelto] from comment #9)
> > When a child process invokes contentPrefService->SetPref() the call is
> > delivered via IPC to the parent which sets the preference accordingly
> 
> Oh ... that's not supposed to work :(.  Child processes shouldn't be able to
> set global prefs.

And it doesn't, except for the whitelisted ones.
This is a revised version of the existing patch with a small hack designed to make sure that the child process setting the pref gets notified of the change (plus a couple of other small fixes). It's far from perfect in the sense that this only fixes our particular problem of the setting child process not being notified but it does not fix the generic problem of notifying all child processes listening on a particular content pref.

I would prefer solving the problem of generic parent->child notifications in a dedicated bug because we could re-use it for removing a lot of ad-hoc code we wrote to pass around notifications via IPC (low-memory, preference changes, etc... just take a look at ContentParent/ContentChild.cpp to get an idea).
Attachment #696281 - Attachment is obsolete: true
This is the same patch but modified to apply and build cleanly on mozilla-b2g18. I tested its functionality on my Otoro and it seems to work as expected.
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
Soft blocker now that bug 821980 is a soft blocker.
blocking-basecamp: + → -
tracking-b2g18: --- → +
This updated patch makes the nsContentPrefsService forward preference change notifications (set/remove) to the child processes and triggers child observers correctly. I would have liked adding a unit test for it but I'm not skilled enough with xpcshell to make it work properly; I have however verified manually that the patch is correctly forwarding the notifications in the child processes.
Attachment #697133 - Attachment is obsolete: true
Attachment #698701 - Flags: review?(ehsan)
This patch is a revised version of the original by Alexandre with minor fixes and a small hack added to prevent the pop-up from showing twice in a row even if the user has dismissed it. The hack is required because if the user requests a long-running script to continue then the content preference adjustment will be delivered *after* the pop-up has showed up again (and thus the event loop has a chance to spin once more).
Attachment #698705 - Flags: review?(ehsan)
Comment on attachment 698701 [details] [diff] [review]
[PATCH 1/2] Forward content preference change notifications to child processes

Review of attachment 698701 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not the right reviewer for any of these patches.
Attachment #698701 - Flags: review?(ehsan)
Attachment #698705 - Flags: review?(ehsan)
(In reply to :Ehsan Akhgari from comment #17)
> I'm not the right reviewer for any of these patches.

Sorry for the inconvenience, the log showed your review on a number of commits and I assumed you had previously dealt with this code.
Comment on attachment 698701 [details] [diff] [review]
[PATCH 1/2] Forward content preference change notifications to child processes

Review of attachment 698701 [details] [diff] [review]:
-----------------------------------------------------------------

Using the content pref service seems sort of weird to me, not sure why we don't just whitelist certain prefs in the pref service in this way but I guess if we have to go this route...

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +44,5 @@
>        // messages. The whitelist contains only those settings that
>        // are not at risk for either.
>        // We currently whitelist saving/reading the last directory of file
>        // uploads, and the last current spellchecker dictionary which are so far
>        // the only need we have identified.

Should add to this comment.

@@ +371,3 @@
>      this._notifyPrefSet(group, aName, aValue);
> +    this.messageManager.broadcastAsyncMessage('ContentPref:notifyPrefSet',
> +      { "group": group, "name": aName, "value": aValue } );

If we're concerned about compromised child processes tracking content prefs then we really need to only send these messages when the pref matches the whitelist above. I suggest making the whitelist constant global and probably renaming to REMOTE_WHITELIST to make it more obvious what it is.

@@ +424,4 @@
>      this._cache.remove(group, aName);
>      this._notifyPrefRemoved(group, aName);
> +    this.messageManager.broadcastAsyncMessage('ContentPref:notifyPrefRemoved',
> +      { "group": group, "name": aName } );

Ditto
Attachment #698701 - Flags: review-
Comment on attachment 698705 [details] [diff] [review]
[PATCH 2/2] Turn the nsJSEnvironment prefs into content prefs and prevent the slow script dialog from showing up after having been permanently dismissed

Review of attachment 698705 [details] [diff] [review]:
-----------------------------------------------------------------

Not really sure who is an appropriate reviewer for this patch, maybe jst can point it in the right direction.

::: dom/base/nsJSEnvironment.cpp
@@ +288,5 @@
> +NS_IMETHODIMP
> +nsContentPrefObserver::OnContentPrefSet(const nsAString& aGroup,
> +                                        const nsAString& aName,
> +                                        nsIVariant* aValue)
> +{

Might want to verify that aGroup is null, or whatever it is meant to be for global prefs, as we should never get this on a per-site basis.
Attachment #698705 - Flags: review?(jst)
Comment on attachment 698705 [details] [diff] [review]
[PATCH 2/2] Turn the nsJSEnvironment prefs into content prefs and prevent the slow script dialog from showing up after having been permanently dismissed

Looks good, r=jst
Attachment #698705 - Flags: review?(jst) → review+
I've modified the patch according to your review:

- The white list is now a global, I have renamed it and added to the comment
- Notifications of non-whitelisted preferences are not broadcast to child processes
- Fixed another place where observers were being notified only locally (in removePrefsByName())
Attachment #698701 - Attachment is obsolete: true
Attachment #699109 - Flags: review?(dtownsend+bugmail)
(In reply to Dave Townsend (:Mossop) from comment #21)
> Using the content pref service seems sort of weird to me, not sure why we
> don't just whitelist certain prefs in the pref service in this way but I
> guess if we have to go this route...

Yes, the current solution is not exactly optimal and providing whitelisted global preferences is probably a better idea. I would like to make a follow up to this with a better designed solution for this problem but in the meantime we need this fix in B2G; so if it's not too bad I'd like to leave it as it is.
(In reply to Dave Townsend (:Mossop) from comment #21)
> @@ +371,3 @@
> >      this._notifyPrefSet(group, aName, aValue);
> > +    this.messageManager.broadcastAsyncMessage('ContentPref:notifyPrefSet',
> > +      { "group": group, "name": aName, "value": aValue } );
> 
> If we're concerned about compromised child processes tracking content prefs
> then we really need to only send these messages when the pref matches the
> whitelist above. I suggest making the whitelist constant global and probably
> renaming to REMOTE_WHITELIST to make it more obvious what it is.

This doesn't sound right to me. I don't see why we would want a situation where a content process has a different view of site-specific preferences than the chrome process.
Try run for 4ab7f5b444da is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4ab7f5b444da
Results (out of 19 total builds):
    success: 13
    warnings: 3
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gsvelto@mozilla.com-4ab7f5b444da
(In reply to Josh Matthews [:jdm] from comment #29)
> (In reply to Dave Townsend (:Mossop) from comment #21)
> > @@ +371,3 @@
> > >      this._notifyPrefSet(group, aName, aValue);
> > > +    this.messageManager.broadcastAsyncMessage('ContentPref:notifyPrefSet',
> > > +      { "group": group, "name": aName, "value": aValue } );
> > 
> > If we're concerned about compromised child processes tracking content prefs
> > then we really need to only send these messages when the pref matches the
> > whitelist above. I suggest making the whitelist constant global and probably
> > renaming to REMOTE_WHITELIST to make it more obvious what it is.
> 
> This doesn't sound right to me. I don't see why we would want a situation
> where a content process has a different view of site-specific preferences
> than the chrome process.

We already have that, the content prefs that content processes can access are controlled by a whitelist, see the comment at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/nsContentPrefService.js#33
Comment on attachment 699109 [details] [diff] [review]
[PATCH 1/2 v2] Forward content preference change notifications to child processes

Review of attachment 699109 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the below change, I don't need to see it again.

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +380,5 @@
>  
> +    if (REMOTE_WHITELIST.indexOf(aName) != -1) {
> +      this.messageManager.broadcastAsyncMessage('ContentPref:notifyPrefSet',
> +        { "group": group, "name": aName, "value": aValue } );
> +    }

You're also missing another _notifyPrefSet above (new since private browsing changes landed). Let's make sure we catch them all, instead of putting chinks like this beside every _notifyPrefSet, just put this code inside the _notifyPrefSet function. Same for _notifyPrefRemoved.
Attachment #699109 - Flags: review?(dtownsend+bugmail) → review+
(In reply to Dave Townsend (:Mossop) from comment #32)
> You're also missing another _notifyPrefSet above (new since private browsing
> changes landed).

Actually there's more spots in the file related to private browsing instances but I had left them alone because when in private browsing mode the changes are limited to the process generating them and thus should not be published to listeners in other processes. Is my assumption wrong?

> Let's make sure we catch them all, instead of putting
> chinks like this beside every _notifyPrefSet, just put this code inside the
> _notifyPrefSet function. Same for _notifyPrefRemoved.

Yes, since I'm using _notifyPrefSet/Removed() in the child and I don't want them to broadcast the changes I'll add two more methods for the parent that broadcast the message and then invoke _notifyPrefSet/Removed().
Revised patch with the changes from comment 32; I've also added some documentation to clarify what's going on.
Attachment #699109 - Attachment is obsolete: true
Revised the patch with a fix for an issue during shutdown that caused debug builds to fail.
Attachment #699120 - Attachment is obsolete: true
(In reply to Gabriele Svelto [:gsvelto] from comment #33)
> (In reply to Dave Townsend (:Mossop) from comment #32)
> > You're also missing another _notifyPrefSet above (new since private browsing
> > changes landed).
> 
> Actually there's more spots in the file related to private browsing
> instances but I had left them alone because when in private browsing mode
> the changes are limited to the process generating them and thus should not
> be published to listeners in other processes. Is my assumption wrong?

It depends on how multiprocess is working, if you can end up with the same domain in two different processes then I can see it may have to be passed between processes still. That may not be possible right now but I'd rather have all this working consistently. The latest patch looks good to land to me.
(In reply to Dave Townsend (:Mossop) from comment #38)
> It depends on how multiprocess is working, if you can end up with the same
> domain in two different processes then I can see it may have to be passed
> between processes still. That may not be possible right now but I'd rather
> have all this working consistently. The latest patch looks good to land to
> me.

Perfect, thanks for your review and your help. I'll attach one final version of this patch in which I have fixed a further issue that cropped up while running xpcshell unit tests (a typo in a parameter). I already passed it through the try servers and I've got all greens except for some test failures in mozRemoteConnection.cpp which seem unrelated:

https://tbpl.mozilla.org/?tree=Try&rev=fd5a4bfe149c
Fixed a typo in a parameter
Attachment #699775 - Attachment is obsolete: true
This is ready for check-in.
Keywords: checkin-needed
The b2g18 versions need approval before they can be landed, right?
Also, it looks like comment 39's Try run has xpcshell orange w/ the same set of failures on every debug platform:
https://tbpl.mozilla.org/php/getParsedLog.php?id=18641109&tree=Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=18639772&tree=Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=18639977&tree=Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=18639515&tree=Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=18642769&tree=Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=18643188&tree=Try
... and those runs were green on the Try run's parent-revision m-c's TBPL, so this wasn't just a case of being based off of a busted m-c revision:
  https://tbpl.mozilla.org/?rev=be151be0cf60

So, it looks like we need to sort out those Try xpcshell oranges before this can land.
  --> removing checkin-needed.
Keywords: checkin-needed
Version: unspecified → Trunk
(In reply to Daniel Holbert [:dholbert] from comment #44)
> So, it looks like we need to sort out those Try xpcshell oranges before this
> can land.

I've been looking into this since last night and I still haven't figured out what's going on. Without my patches applied the test runs but spits out these warnings:

JS Component Loader: ERROR /home/gsvelto/projects/mozilla-central/testing/xpcshell/head.js:775
                     TypeError: Components is undefined
JS Component Loader: ERROR /home/gsvelto/projects/mozilla-central/testing/xpcshell/head.js:775
                     TypeError: Components is undefined
JS Component Loader: ERROR /home/gsvelto/projects/mozilla-central/testing/xpcshell/head.js:775
                     TypeError: Components is undefined
resource:///modules/webappsUI.jsm:26: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]
WARNING: nsExceptionService ignoring thread destruction after shutdown: file /home/gsvelto/projects/mozilla-central/xpcom/base/nsExceptionService.cpp, line 166
WARNING: NS_ENSURE_TRUE(thread) failed: file /home/gsvelto/projects/mozilla-central/netwerk/base/src/nsSocketTransportService2.cpp, line 115
WARNING: unable to post SHUTDOWN message: file /home/gsvelto/projects/mozilla-central/netwerk/protocol/http/nsHttpConnectionMgr.cpp, line 150
WARNING: An event was posted to a thread that will never run it (rejected): file /home/gsvelto/projects/mozilla-central/xpcom/threads/nsThread.cpp, line 368
WARNING: Failed dispatching block-event: file /home/gsvelto/projects/mozilla-central/netwerk/cache/nsCacheService.cpp, line 855
WARNING: NS_ENSURE_TRUE(compMgr) failed: file /home/gsvelto/projects/build/firefox-debug/xpcom/build/nsComponentManagerUtils.cpp, line 58
WARNING: OOPDeinit() without successful OOPInit(): file /home/gsvelto/projects/mozilla-central/toolkit/crashreporter/nsExceptionHandler.cpp, line 2274

Now with my patches applied if you run the test in interactive mode (|make SOLO_FILE="test_421483.js" -C $objdir/browser/components/places/tests/ check-one|) you'll see that the test runs fine through _execute_test() but then once you invoke quit() it crashes during shutdownwith the following messages:

JS Component Loader: ERROR /home/gsvelto/projects/mozilla-central/testing/xpcshell/head.js:775
                     TypeError: Components is undefined
JS Component Loader: ERROR /home/gsvelto/projects/mozilla-central/testing/xpcshell/head.js:775
                     TypeError: Components is undefined
JS Component Loader: ERROR /home/gsvelto/projects/mozilla-central/testing/xpcshell/head.js:775
                     TypeError: Components is undefined
resource:///modules/webappsUI.jsm:26: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]
WARNING: nsExceptionService ignoring thread destruction after shutdown: file /home/gsvelto/projects/mozilla-central/xpcom/base/nsExceptionService.cpp, line 166
Assertion failure: !connections[i]->ConnectionReady(), at /home/gsvelto/projects/mozilla-central/storage/src/mozStorageService.cpp:828
<<<<<<<
PROCESS-CRASH | /home/gsvelto/projects/build/firefox-debug/_tests/xpcshell/browser/components/places/tests/unit/test_421483.js | application crashed [Unknown top frame]

I'll have to look into it further but I'm a bit confused; my code isn't causing the crash directly yet it's affecting the test behavior in such a way that it ends up crashing in the end.
Note that it's not really a crash, it's an assertion-failure.  That code is asserting that all our database connections are closed -- but one of them is still open, so the assertion fails.

Presumably the database in question is the Places database, since this is a places test. 

From glancing at the test, it looks like it does a lot of pref setting / getting intermixed with places-DB-tweaks.   Presumably, that exercises the code in this bug's patches, and that ends up blocking something that closes a places DB connection.
(In reply to Daniel Holbert [:dholbert] from comment #46)
> Note that it's not really a crash, it's an assertion-failure.  That code is
> asserting that all our database connections are closed -- but one of them is
> still open, so the assertion fails.

Yes, my bad.

> Presumably the database in question is the Places database, since this is a
> places test.

The content preferences also use a database and calls asyncClose() on the connection. Looking at the code in mozStorageConnection.cpp this schedules a runnable for execution. It might be that my changes somehow prevent that part from executing and leave the connection open.

> From glancing at the test, it looks like it does a lot of pref setting /
> getting intermixed with places-DB-tweaks.   Presumably, that exercises the
> code in this bug's patches, and that ends up blocking something that closes
> a places DB connection.

Thing is, this patch only affect content preferences which are a separate subsystem from regular preferences and from what I could see by placing breakpoints my changes don't seem to be exercised at all except for the startup/shutdown part. My gut feeling is that there's something in how the test shuts down that's interacting with the content prefs shutdown and preventing it from running to completion. That would also explain why when running the test on the unmodified sources the shutdown procedure prints out warnings.
I've done further testing and I'm under the impression that what we're seeing is a flaw in the test harness or in the test itself which I'll try to address. My suspect that something weird is going on during shutdown was confirmed today when I've reproduced the bug on an unmodified mozilla-central clone with only this line added to nsJSRuntime::Init():

nsCOMPtr<nsIContentPrefService> contentPrefService = do_GetService(NS_CONTENT_PREF_SERVICE_CONTRACTID);

This initializes the content preference service which will open a db connection; the service will then be left unused. During shutdown the normal cleanup procedure of the service is not invoked, this in turn causes the db connection not to be closed and triggers the assertion.
OK, I've figured out what's going wrong and why. In all the tests under the browser/components/places/tests/unit folder nsJSEnvironment::Init() is called after mozilla::ShutdownXPCOM() has been called.

This causes the following: nsContentPrefService starts and subscribes to the xpcom-shutdown event, it will use this event to close its db connection. Since XPCOM shutdown is already undergoing the xpcom-shutdown notification will never be delivered to nsContentPrefService and so the db connection will be left open. This later triggers the assertion in nsMozStorageService.

Unfortunately I have no idea of why XPCOM is behaving in such a weird way in these tests and I will need some help to sort it out.
Figuring out that test failure was all but trivial and it became even more confusing when the tests started to pass even with my unmodified patches. Turns out that the problem is related to how the browser glue service was being shut down in the xpcshell tests under browser/components/places/tests/unit.

The issue was caused by nsBrowserGlue trying to tear down some services (webrtc, webapps, ...) that it was supposed to have initialized when the profile had been loaded. In the affected tests the profile was never loaded so nsBrowserGlue tried to shut down services that had never been initialized in the first place. This triggered an error which a new JS runtime to be instanced while the XPCOM shutdown was already undergoing and eventually triggered the assertion as I described above.

Somehow this last issue has disappeared from mozilla-central due to some recent commit but you can still see a bunch of warnings during these tests because of nsBrowserGlue not shutting down properly.

This patch should fix the issue with nsBrowserGlue and a unit-test which was relying on its peculiar shutdown behavior. I've submitted the whole set to try, if it goes green I'll refresh all patches and request the reviews again though I wonder if it wouldn't be better to open a separate issue for the nsBrowserGlue problem:

https://tbpl.mozilla.org/?tree=Try&rev=3d0a02070dd2
Refreshed patch, refactored to better match the existing code and coding conventions. All the type declarations in the header file were moved into the .cpp file to make the patch less intrusive.
Attachment #699778 - Attachment is obsolete: true
Fixes the issues that cropped up in the places tests and quiets a few warnings which were thrown even without the other patches applied due to the nsBrowserGlue improper shutdown.
Attachment #702408 - Attachment is obsolete: true
Requesting review again because this needs to land on mozilla-b2g18 too. The code is essentially unchanged except for the necessary adaptation to make it apply cleanly.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 820438
User impact if declined: The dialog to block long-running scripts cannot be permanently dismissed
Testing completed: Tested on an Otoro device + try run (see below)
Risk to taking this patch (and alternatives if risky): Existing content preference behavior will change for child processes though only two clients will be affected (nsJSRuntime and the browser FullZoom property) and they should both accept the new behavior correctly as the old behavior was buggy
String or UUID changes made by this patch: none
Attachment #700126 - Attachment is obsolete: true
Attachment #702867 - Flags: review?(dtownsend+bugmail)
Attachment #702867 - Flags: approval-mozilla-b2g18?
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Requesting another review because this needs to land on mozilla-b2g18 too. The code functionality is unchanged but I've restricted the changes to the .cpp files in order to make the patch less intrusive.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 820438
User impact if declined: The dialog to block long-running scripts cannot be permanently dismissed
Testing completed: Tested on an Otoro device + try run (see below)
Risk to taking this patch (and alternatives if risky): nsJSRuntime now depends on the content preferences service, this might cause interactions with other components in certain corner cases (see the issue with nsBrowserGlue describved in comment 50)
String or UUID changes made by this patch: none
Attachment #699781 - Attachment is obsolete: true
Attachment #702870 - Flags: review?(jst)
Attachment #702870 - Flags: approval-mozilla-b2g18?
Asking for review for the third patch as it wasn't part of the original set. This fixes the side-effect of this patch-set described in comment 50.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 820438
User impact if declined: None, issues show up only in the test suite
Testing completed: Tested on an Otoro device + try run (see below)
Risk to taking this patch (and alternatives if risky): The patch cause issues during application shutdown though it is unlikely
String or UUID changes made by this patch: none
Attachment #702874 - Flags: review?(mak77)
Attachment #702874 - Flags: review?(gavin.sharp)
Attachment #702874 - Flags: approval-mozilla-b2g18?
Attachment #702867 - Flags: review?(dtownsend+bugmail) → review+
Comment on attachment 702870 [details] [diff] [review]
[PATCH 2/3 v5 mozilla-b2g18] Turn the nsJSEnvironment prefs into content prefs and prevent the slow script dialog from showing up after having been permanently dismissed

- In nsContentPrefObserver::OnContentPrefSet():

+  // Default limit on script run time to 10 seconds. 0 means let
+  // scripts run forever.
+  bool isChromePref = aName.Equals(CPS_CHROME_PREF_NAME);
+  int32_t time = isChromePref ? 2 : 1;

That should be 20 : 10 instead of 2 : 1.

- In nsJSContext::DOMOperationCallback():

+      // HACK: Set the pref immediately to prevent the dialog popping up again
+      // in child processes before the pref change event is delivered.
+       if (isTrackingChromeCodeTime) {
+         sMaxChromeScriptRunTime = 0x40000000LL << 32;
+       } else {
+         sMaxScriptRunTime = 0x40000000LL << 32;
+       }

Please fix the indentation of this code, one too many spaces here.

Please make the same changes in both the mozilla-central and b2g18 versions of the patch, I overlooked both of these problems in my first review of this code :(
Attachment #702870 - Flags: review?(jst) → review+
Added the changes in comment 58. Thank you for spotting these issues Johnny, after refreshing the patch set many times I got careless :-(
Attachment #702862 - Attachment is obsolete: true
See comment 55 for the approval request for mozilla-b2g18.
Attachment #702870 - Attachment is obsolete: true
Attachment #702870 - Flags: approval-mozilla-b2g18?
Attachment #703225 - Flags: approval-mozilla-b2g18?
Attachment #702874 - Flags: review?(gavin.sharp)
I'm a bit worried by these changes cause nsIContentPrefService is deprecated and we plan to convert all of its consumers and remove it.
nsIContentPrefService2 is the replacement, and it uses a different code that lives in ContentPrefService2.jsm that means the changes in nsContentPrefService.js will be lost and we have another consumer to convert.
One way to deal with this could be to land the b2g18 patches only for which we have no option of switching to another client and then open a separate bug to fix the issue on central in nsIContentPrefService2. I'm willing to do that as long as we can fix the problem on B2G too.
Comment on attachment 702874 [details] [diff] [review]
[PATCH 3/3 v5 mozilla-b2g18] Tear down components initialized during profile startup only when a profile has been really loaded

Review of attachment 702874 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserGlue.js
@@ +232,5 @@
> +        }
> +        else {
> +          // places-shutdown is fired when the profile is about to disappear.
> +          this._onProfileShutdown();
> +        }

So, the shutdown is a bit fragile/complicated since we have only a a couple notifications to use and some stuff to properly serialize.

here _onProfileShutdown is invoked by places-shutdown cause, before Places goes away, we need to do some work, among which clear history on shutdown, remove observers from PageThumbs... More generally, any service that is inited and depends on Places needs to be finalized at this topic.

So, I think that you may avoid this hacky data change, by doing these changes to nsBrowserGlue:

1. rename _shutdownPlaces to _onPlacesShutdown
2. Move this._sanitizer.onShutdown(); and PageThumbs.uninit(); at the beginning of _onPlacesShutdown (they use Places and thus depend on its notifications)
3. add a profile-before-change observer in nsBrowserGlue
4. on receiving places-shutdown call _onPlacesShutdown
5. on receiving profile-before-change call _onProfileShutdown (will happen after places-shutdown, so basically nothing changes)
6. clearly _onProfileShutdown should not invoke anymore _onPlacesShutdown so remove that call

At this point the test should properly only shutdown the Places dependent part of the glue.
Attachment #702874 - Flags: review?(mak77) → review-
(In reply to Gabriele Svelto [:gsvelto] from comment #62)
> One way to deal with this could be to land the b2g18 patches only for which
> we have no option of switching to another client and then open a separate
> bug to fix the issue on central in nsIContentPrefService2. I'm willing to do
> that as long as we can fix the problem on B2G too.

yes, I'm not asking to throw away all of this, also considered the short time we have. We should have catched this issue earlier.
I'm asking to create a short-term plan to ensure this works in nsIContentPrefService2 and change nsJSEnvironment.cpp to use the new interface. File a bug, be sure to properly point out everything that should be converted.
Ah, just a side note, the fact contentprefservice uses xpcom-shutdown is a bug, please file it. Nothing should try to do anything on a profile after profile-before-change, especially if it uses a database.
Comment on attachment 702867 [details] [diff] [review]
[PATCH 1/3 v5 mozilla-b2g18] Forward content preference change notifications to child processes

Given how late this is the risk involved in taking these 3 patches we're not approving these patches for b2g18 at this time. If there are benefits to taking this beyond the slow script dialog working as expected then please re-nominate and we'll re-consider this.
Attachment #702867 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18-
Attachment #702874 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18-
Attachment #703225 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18-
Depends on: 835352
Since while working on this bug I unearthed a bunch of unrelated issues I'm taking those out in separate bugs and I will be fixing them separately in order to make landing this on m-c a less complicated task. See bug 835352, bug 835355 and bug 727996.
In the light of bug 777196 and after some discussion with :cjones I've come to the realization that setting the preference globally is not the right approach to fix this bug. The issue is that doing so will essentially allow the browser app (or any other content process) to globally alter the maximum runtime script for *all other processes*.

A saner solution for this would probably involve extending mozbrowser to allow a browser app to set these parameters for itself alone. I would like to explore this option further down the line and I'll probably open a related bug.

In the meantime I'm proposing this very small and non-intrusive patch to fix the issue at hand. The idea here is to set directly the static variables responsible for the maximum script runtimes instead of waiting for the Preferences callback to set them.

In the main process this will work as usual (the preference will be set and the value changed); in content processes however the preference will not be set but the process' value will be updated thus preventing the pop-up from appearing again.

One side-effect of fixing the issue this way is that a content process will not be able to persist this change across runs. I don't see this as an issue because on FxOS the user wouldn't have a way to change this preference back if he wanted to. Having the value reset to its default every time the user restart the browser app is thus probably a safer behavior.

Additionally I think that this patch is small and risk-less enough to be applied to mozilla-b2g18 fixing the issue there too. I'll wait for review before asking for b2g18 approval though.

I've moved all other issues that cropped up while fixing this bug in other more appropriate bugs and I'm thus obsoleting all other patches.
Attachment #702858 - Attachment is obsolete: true
Attachment #702863 - Attachment is obsolete: true
Attachment #702867 - Attachment is obsolete: true
Attachment #702874 - Attachment is obsolete: true
Attachment #703224 - Attachment is obsolete: true
Attachment #703225 - Attachment is obsolete: true
Attachment #708649 - Flags: review?(jst)
Attachment #708649 - Flags: review?(jst) → review+
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 820438
User impact if declined: The dialog to block long-running scripts was permanently disabled as a workaround in bug 817230, if re-enabled it cannot be permanently dismissed
Testing completed: Tested on an Otoro device + try run (see below)
Risk to taking this patch (and alternatives if risky): No major risk involved, the overall behavior is unchanged except that the patch prevents the pop-up from reappearing when permanently dismissed
String or UUID changes made by this patch: none

Try run: https://tbpl.mozilla.org/?tree=Try&rev=a3d7fa9cfa57
Attachment #708972 - Flags: review?(jst)
Attachment #708972 - Flags: approval-mozilla-b2g18?
I'd like to re-nom this because the way the issue has been worked around makes certain web-pages unusable and the proposed fix is small and low-risk.

As an example of the impact of this bug I find browsing the webpage of a major Italian paper impossible (http://www.repubblica.it). Once on the page a script starts running and never stops. This not only makes that particular site impossible to browse but since there's no way to stop the script it forces you to kill the browser if you want to navigate to any another page.
blocking-b2g: --- → tef?
We don't have to block here, it's already tracking and once the patch is reviewed we'll take a look at this in approval triage - the risk assessment looks good and we'll probably take it for 1.0.1 uplift.
blocking-b2g: tef? → -
We're still waiting on r+ here.
Comment on attachment 708972 [details] [diff] [review]
[PATCH mozilla-b2g18] Immediately apply changes in maximum script runtime to make them visible in content processes

+// Large value used to specify that a script should run essentially forever
+#define NS_UNLIMITED_SCRIPT_RUNTIME LL_INIT(0x40000000, 0)

No need for LL_INIT here any more, you should be able to simply just make this be 0x40000000.

r=jst
Attachment #708972 - Flags: review?(jst) → review+
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #74)
> No need for LL_INIT here any more, you should be able to simply just make
> this be 0x40000000.

I noticed existing code in b2g18 used LL_INIT(0x40000000, 0) so I used the same to play it safe; I can change it though before commit. I've dropped LL_INIT() from the mozilla-central patch which uses 0x40000000LL << 32 instead.
Comment on attachment 708972 [details] [diff] [review]
[PATCH mozilla-b2g18] Immediately apply changes in maximum script runtime to make them visible in content processes

Approving this now that it has review, please update both status flags - you can just land to mozilla-b2g18 tip between now and when we branch v1.0.1 on Wednesday, after that you'll have to double-land.
Attachment #708972 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
(In reply to Lukas Blakk [:lsblakk] from comment #76)
> Approving this now that it has review, please update both status flags - you
> can just land to mozilla-b2g18 tip between now and when we branch v1.0.1 on
> Wednesday, after that you'll have to double-land.

I see you've already updated the status flags so I'll just add checkin-needed. To avoid confusion attachment 708649 [details] [diff] [review] needs to land on mozilla-central and attachment 708972 [details] [diff] [review] on mozilla-b2g18.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/702007f2f745
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.