Fix dangerous pointer use in UserScriptMaster

UserScriptMaster::ScriptReloader chooses to make a plain pointer to a
UserScriptList from UserScriptMaster. This leads to problems:
- What if master goes away?
- What if the UserScriptList is modified, as in
  UserScriptMaster::OnExtensionLoaded?

Fix this by making the script loading process in UserScriptMaster much simpler:
UserScriptMaster calls LoadScriptsOnFile thread, and uses the result, using a
weak pointer.

Added bonus: Two fewer ref-counted objects!  50 fewer lines of code!  Less script loading!

BUG=383973
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282361

[Devlin, 2014-07-07 16:07:27]

Antony, mind taking a look?

https://codereview.chromium.org/362343006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/user_script_master.cc (left):

https://codereview.chromium.org/362343006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:60: bool
UserScriptMaster::ScriptReloader::ParseMetadataHeader(
This function has not changed, but Rietveld's bad at displaying "moves".

https://codereview.chromium.org/362343006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/user_script_master.h (right):

https://codereview.chromium.org/362343006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.h:47: static bool
ParseMetadataHeader(const base::StringPiece& script_text,
This really doesn't belong here - UserScriptMaster doesn't use it.  But I'll
save that for a later CL.

[asargent_no_longer_on_chrome, 2014-07-08 20:59:13]

typo in CL description: "chooses to take a make a ..." [kind of sounds like a
card game: "take a make a" =) ]


Nice improvement! Mostly lg but with one question in user_script_master.cc.

https://codereview.chromium.org/362343006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/user_script_master.cc (right):

https://codereview.chromium.org/362343006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:375:
user_scripts_.reset(user_scripts.release());
In the CL as is, you're passing a copy of user_scripts_ to
LoadScriptsOnFileThread inside StartLoad, and then take that copy back here and
replace the original contents of user_scripts_ with it. Can we just pass
ownership over to the file thread instead of making the copy? I guess you'd need
to slightly rework the OnExtensionLoaded / OnExtensionUnloaded so that instead
of modifying user_scripts_, you instead kept track of UserScript objects to add
to / remove from the list that will be coming back from the file thread.

[Devlin, 2014-07-09 16:03:25]

https://codereview.chromium.org/362343006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/user_script_master.cc (right):

https://codereview.chromium.org/362343006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:375:
user_scripts_.reset(user_scripts.release());
On 2014/07/08 20:59:13, Antony Sargent wrote:
> In the CL as is, you're passing a copy of user_scripts_ to
> LoadScriptsOnFileThread inside StartLoad, and then take that copy back here
and
> replace the original contents of user_scripts_ with it. Can we just pass
> ownership over to the file thread instead of making the copy? I guess you'd
need
> to slightly rework the OnExtensionLoaded / OnExtensionUnloaded so that instead
> of modifying user_scripts_, you instead kept track of UserScript objects to
add
> to / remove from the list that will be coming back from the file thread. 
> 

Yeah, I was trying to keep the underlying mechanism relatively similar for
safety.  But let's see what happens if we tweak it a bit. =)

As you mentioned, this required updating the OnExtension[Un]loaded to track
which extensions were added/removed.  On the plus side, this allows for an extra
implementation speed-up - we can now only load the _new_ extensions, instead of
reloading them all.  An extra pair of eyes to make sure this is all still sound
would be appreciated. :)

[asargent_no_longer_on_chrome, 2014-07-09 22:08:20]

nice, this seems better

lgtm

[Devlin, 2014-07-10 15:19:08]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-07-10 15:19:56]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/362343006/...

[commit-bot: I haz the power, 2014-07-10 17:08:05]

Change committed as 282361