Closed Bug 1394578 Opened 7 years ago Closed 7 years ago

PREF_PrefIsLocked not working properly with e10s

Categories

(Core :: Preferences: Backend, defect)

Product:
Core
Component:
Preferences: Backend
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: mkaply, Assigned: n.nethercote)

References

Details

Attachments

(10 files)

Bug 1394578 - Add PrefHashEntry::ValueMatches().
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1394578 - Rename dom::PrefSetting as dom::Pref.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1394578 - Use gHashTable->EntryCount() to size aDomPrefs, not aCapacity().
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1394578 - PrefHashEntry as Pref.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1394578 - Remove the kPref* enum.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1394578 - Move more operations into PrefValue.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1394578 - Move Pref::AssignPrefValueToDomPrefVAlue() to PrefValue::ToDomPrefValue().
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1394578 - Introduce PrefType::None.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1394578 - Rewrite Preferences::SetPreference().
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1394578 - Pass pref locked status to content processes.
59 bytes, text/x-review-board-request
glandium
: review+
Details 
I have no explanation for what is going on here. See bug 1359837.

With e10 enabled, querying if a pref is locked is not working from AddonManagerWebAPI.cpp.

Even though gIsAnyPrefLocked got set to true when I lock the pref using this syntax in the browser scratchpad:

var prefs = Services.prefs.getDefaultBranch(null); 
prefs.setBoolPref("xpinstall.enabled", false);
prefs.lockPref("xpinstall.enabled");

when prefs->PrefIsLocked("xpinstall.enabled", &isLocked); is called, it returns false.

Debugging shows that gIsAnyPrefLocked is false in this case, even though it got set to true when it was locked.

I'm guessing the problem is that the API is going across processes, but this should be working just like getting the value of a preference - it should work across process.

But even if I remove the gIsAnyPrefLocked check, it still fails. (Actually it breaks locking of prefs in general)
Blocks: 1359837
Component: Preferences → Preferences: Backend
Product: Toolkit → Core
> I'm guessing the problem is that the API is going across processes, but this
> should be working just like getting the value of a preference - it should
> work across process.

I was looking at the cross-process prefs code today. AFAICT, the locked/unlocked status of prefs simply isn't communicated to child processes. Which means that all prefs will always appear unlocked within child processes.

Also, all methods that modify prefs fail when run in a child process. This includes lockPref().
(In reply to Nicholas Nethercote [:njn] from comment #1)
> I was looking at the cross-process prefs code today. AFAICT, the
> locked/unlocked status of prefs simply isn't communicated to child
> processes. Which means that all prefs will always appear unlocked within
> child processes.

Yeah, when they did e10s for prefs, they just left this out. I looked into the possibility of fixing it, but I was completely lost on how the child/parent pref things work.
Mike: I'm happy to fix this, but I'd like to understand the use case first. Prefs can't be modified in content processes, so even if I add the ability to correctly query the locked status in a content process, you won't be able to lock a pref from a content process. Is that a problem?
Flags: needinfo?(mozilla)
The use case is bug 1359837. Trying to check the state of a pref in order to disallow Test Pilot for enterprise.

Only need to read the state, not set it.

thanks!
Flags: needinfo?(mozilla)
Comment on attachment 8931525 [details]
Bug 1394578 - Introduce PrefType::None.

https://reviewboard.mozilla.org/r/202644/#review207946


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: modules/libpref/Preferences.cpp:278
(Diff revision 1)
>  static ArenaAllocator<8192, 1> gPrefNameArena;
>  
>  class Pref : public PLDHashEntryHdr
>  {
>  public:
> -  Pref(const char* aName, PrefType aType)
> +  Pref(const char* aName)

Error: Bad implicit conversion constructor for 'pref' [clang-tidy: mozilla-implicit-constructor]
The updated patches now pass all tests on try.
Comment on attachment 8931518 [details]
Bug 1394578 - Add PrefHashEntry::ValueMatches().

https://reviewboard.mozilla.org/r/202630/#review208784

::: commit-message-4c24f:4
(Diff revision 1)
> +Bug 1394578 - Add PrefHashEntry::ValueMatches(). r=glandium
> +
> +This factors out some common code from SetValue(), making it easier to read.
> +The pach also improves the comments in SetValue().

typo: patch

::: modules/libpref/Preferences.cpp:474
(Diff revision 1)
>      }
>    }
>  
> +  bool ValueMatches(PrefValueKind aKind, PrefType aType, PrefValue aValue)
> +  {
> +    return (aKind == PrefValueKind::Default)

this could also be written as:
  IsType(aType) && (aKind == PrefValueKind::Default) ? ... : ...

::: modules/libpref/Preferences.cpp:555
(Diff revision 1)
> +    if (mHasUserValue &&
> +        (!ValueMatches(PrefValueKind::Default, Type(), mUserValue) ||

Note: this is kind of confusing at first glance, compared to the original code, but I think it's not too bad.
Attachment #8931518 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8931519 [details]
Bug 1394578 - Rename dom::PrefSetting as dom::Pref.

https://reviewboard.mozilla.org/r/202632/#review208788
Attachment #8931519 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8931520 [details]
Bug 1394578 - Use gHashTable->EntryCount() to size aDomPrefs, not aCapacity().

https://reviewboard.mozilla.org/r/202634/#review208790
Attachment #8931520 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8931521 [details]
Bug 1394578 - PrefHashEntry as Pref.

https://reviewboard.mozilla.org/r/202636/#review208792
Attachment #8931521 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8931522 [details]
Bug 1394578 - Remove the kPref* enum.

https://reviewboard.mozilla.org/r/202638/#review208796
Attachment #8931522 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8931522 [details]
Bug 1394578 - Remove the kPref* enum.

https://reviewboard.mozilla.org/r/202638/#review208798

Is this the last bit conflicting with the patch in bug 440908?
Comment on attachment 8931523 [details]
Bug 1394578 - Move more operations into PrefValue.

https://reviewboard.mozilla.org/r/202640/#review208802

::: modules/libpref/Preferences.cpp:526
(Diff revision 1)
>  
>          // Otherwise, should we set the user value? Only if doing so would
>          // change the user value.
>        } else if (!ValueMatches(PrefValueKind::User, aType, aValue)) {
> -        ReplaceValue(PrefValueKind::User, aType, aValue);
> +        mUserValue.Replace(Type(), aType, aValue);
> +        SetType(aType);

Maybe add a comment that this is needed because there ight be no default value, so no type?
Attachment #8931523 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8931524 [details]
Bug 1394578 - Move Pref::AssignPrefValueToDomPrefVAlue() to PrefValue::ToDomPrefValue().

https://reviewboard.mozilla.org/r/202642/#review208808

::: modules/libpref/Preferences.cpp:407
(Diff revision 1)
>        aDomPref->defaultValue() = dom::PrefValue();
> -      AssignPrefValueToDomPrefValue(
> -        Type(), &mDefaultValue, &aDomPref->defaultValue().get_PrefValue());
> +      mDefaultValue.ToDomPrefValue(Type(),
> +                                   &aDomPref->defaultValue().get_PrefValue());

Why not make ToDomPrefValue return a PrefValue by value, and just assign it to aDomPref->defaultValue()?
Attachment #8931524 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8931525 [details]
Bug 1394578 - Introduce PrefType::None.

https://reviewboard.mozilla.org/r/202644/#review208810
Attachment #8931525 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8931526 [details]
Bug 1394578 - Rewrite Preferences::SetPreference().

https://reviewboard.mozilla.org/r/202646/#review208814

::: modules/libpref/Preferences.cpp:482
(Diff revision 3)
> +        mHasUserValue = true;
> +        userValueChanged = true;
> +      }
> +    } else if (mHasUserValue) {
> +      ClearUserValue();
> +      userValueChanged = true;

This is the case where the default value changed in a way that makes the user value match it, right? Then there actually is no value change, isn't there?
Attachment #8931526 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8931527 [details]
Bug 1394578 - Pass pref locked status to content processes.

https://reviewboard.mozilla.org/r/202648/#review208820

::: dom/ipc/ContentParent.cpp:2800
(Diff revision 3)
>      }
>  
>      // We know prefs are ASCII here.
>      NS_LossyConvertUTF16toASCII strData(aData);
>  
> -    Pref pref(strData, null_t(), null_t());
> +    Pref pref(strData, false, null_t(), null_t());

maybe add /* isLocked */ comments for each of those changes?
Attachment #8931527 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #27)
> Comment on attachment 8931522 [details]
> Bug 1394578 - Remove the kPref* enum.
> 
> https://reviewboard.mozilla.org/r/202638/#review208798
> 
> Is this the last bit conflicting with the patch in bug 440908?

Yes. And I have more parser changes coming that will conflict even more. But it's all part of the plan that will allow locked_pref to be integrated, so the short-term pain should be worth it.
I'll wait some more before refreshing the patch there, then.
> > -        ReplaceValue(PrefValueKind::User, aType, aValue);
> > +        mUserValue.Replace(Type(), aType, aValue);
> > +        SetType(aType);
> 
> Maybe add a comment that this is needed because there ight be no default
> value, so no type?

Not quite. It's needed because it's possible to change the type when setting the user value, if there is no default value. I'll add a comment to that effect.
> Why not make ToDomPrefValue return a PrefValue by value, and just assign it
> to aDomPref->defaultValue()?

Two reasons:
- To mirror ToDomPref(), which also assigns via a pointer;
- Because dom::PrefValue is a moderately large type (multiple words) so this is a little cheaper.
(In reply to Nicholas Nethercote [:njn] from comment #36)
> - Because dom::PrefValue is a moderately large type (multiple words) so this
> is a little cheaper.

FWIW, compilers inline those things, effectively making the intermediate value go away.
> > +    } else if (mHasUserValue) {
> > +      ClearUserValue();
> > +      userValueChanged = true;
> 
> This is the case where the default value changed in a way that makes the
> user value match it, right? Then there actually is no value change, isn't
> there?

It's not that case. This code path doesn't know what happened with the default value. The possible scenarios:

- The content process may or may not have a default value for the pref, and
- The content process has a user value for the pref, and
- The parent process overwrites the pref with one of:
  - { default: non_empty, user: empty }
  - { default: empty, user: empty }

No matter what happens, the user value changes (it gets cleared). It's possible that the new default value matches the old user value, in which case we'll needlessly notify, but that's safe and it should be rare.
mkaply: hopefully your original test case works now!
Flags: needinfo?(mozilla)
Works like a charm. Thanks!
Flags: needinfo?(mozilla)
Assignee: nobody → n.nethercote
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: