Closed Bug 1151495 Opened 9 years ago Closed 9 years ago

Support permission prompting from workers for IDB

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- fixed
firefox40 --- fixed
firefox-esr38 --- wontfix

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch worker.patch (obsolete) — Splinter Review
http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/ActorsChild.cpp#1118

Currently this is not supported.
Attachment #8588626 - Flags: review?(bent.mozilla)
Comment on attachment 8588626 [details] [diff] [review]
worker.patch

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

We need a WorkerFeature here too. Any time we do something async there's a possibility that the worker could start being torn down.

::: dom/indexedDB/ActorsChild.cpp
@@ +1119,5 @@
> +class WorkerPermissionChallenge;
> +
> +// This class calles OWorkerPermissionChallenge::perationCompleted() in the
> +// worker thread.
> +class WorkerPermissionOperationCompleted final : public WorkerRunnable

Rather than having all these different classes can we just combine them into one? Then you wouldn't have so many objects with tricky lifetimes, etc.

@@ +1210,5 @@
> +    }
> +
> +    nsPIDOMWindow* window = wp->GetWindow();
> +    if (!window) {
> +      OperationCompleted();

Rather than having to put |OperationCompleted();| at every return why not do this:

  nsresult RunInternal() { /* all your code */ }

  NS_IMETHOD Run()
  {
    nsresult rv = RunInternal();
    
    OperationCompleted();

    return rv;
  }

@@ +1325,5 @@
>    if (!NS_IsMainThread()) {
> +    nsRefPtr<WorkerPermissionChallenge> runnable =
> +      new WorkerPermissionChallenge(this, mFactory, aPrincipalInfo);
> +
> +    if (NS_WARN_IF(NS_FAILED(NS_DispatchToMainThread(runnable)))) {

MOZ_ALWAYS_TRUE(NS_SUCCEEDED(...))
Attachment #8588626 - Flags: review?(bent.mozilla) → review-
The main problem is here is that PermissionRequestBase is fully changed and now it wants an Element* instead a nsPIDOMWindow*). Any idea how to get a valid element in the worker context?
Flags: needinfo?(bent.mozilla)
Oh, yeah, sorry. Blake just changed that for e10s.

So for workers that live in windows in the parent process you do exactly this: http://hg.mozilla.org/mozilla-central/rev/c76e6b6d4591#l2.52

For workers that live in windows in the child process you use |TabChild::GetFrom(window)| and then send the PIndexedDBPermissionRequest message.
Flags: needinfo?(bent.mozilla)
Attached patch worker.patch (obsolete) — Splinter Review
I don't see how we can combine all of these classes in 1:

WorkerPermissionOperationCompleted is a WorkerRunnable and it has it's own Run().

WorkerPermissionRequest inherits from PermissionRequestBase and it wants an dom::Element. For this we need the window, and we must be in the main-thread.

WorkerPermissionRequestChildProcessActor is allocated and then owned by the IPDL protocol.

WorkerPermissionChallenge is the main-class for this patch. It is runnable for the main-thread and WorkerFeature for the worker thread. I could combine it with the first WorkerRunnable but I don't know if this makes the code more readable.
Attachment #8588626 - Attachment is obsolete: true
Attachment #8595151 - Flags: review?(bent.mozilla)
Comment on attachment 8595151 [details] [diff] [review]
worker.patch

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

This looks like an incomplete patch?
Attachment #8595151 - Flags: review?(bent.mozilla)
Attached patch worker.patch (obsolete) — Splinter Review
Attachment #8595151 - Attachment is obsolete: true
Attachment #8595649 - Flags: review?(bent.mozilla)
Comment on attachment 8595649 [details] [diff] [review]
worker.patch

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

This looks great! Thanks! Most of this is just little stuff:

::: dom/indexedDB/ActorsChild.cpp
@@ +1114,5 @@
>  
>    return true;
>  }
>  
> +namespace {

Can you put this stuff in the other anonymous namespace rather than making another?

@@ +1118,5 @@
> +namespace {
> +
> +class WorkerPermissionChallenge;
> +
> +// This class calles OWorkerPermissionChallenge::perationCompleted() in the

Nit: typos here

@@ +1126,5 @@
> +  nsRefPtr<WorkerPermissionChallenge> mChallenge;
> +
> +public:
> +  WorkerPermissionOperationCompleted(WorkerPermissionChallenge* aChallenge,
> +                                     WorkerPrivate* aWorkerPrivate)

Nit: Swap the order of these args so that base class args go first. Same for WorkerPermissionRequest.

@@ +1133,5 @@
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +  }
> +
> +  bool WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate);

Nit: add virtual and override keywords

@@ +1137,5 @@
> +  bool WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate);
> +};
> +
> +// Prompt management in the main-thread.
> +class WorkerPermissionRequest final : public PermissionRequestBase

Nit: Maybe comment that this is the class used to do prompting in the main thread and main process.

@@ +1147,5 @@
> +                          Element* aElement, nsIPrincipal* aPrincipal)
> +    : PermissionRequestBase(aElement, aPrincipal)
> +    , mChallenge(aChallenge)
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());

Also assert that this is the main process?

@@ +1161,5 @@
> +  virtual void
> +  OnPromptComplete(PermissionValue aPermissionValue) override;
> +};
> +
> +class WorkerPermissionRequestChildProcessActor final

Nit: And then comment here that this is used in the main thread of all child processes.

@@ +1171,5 @@
> +  explicit WorkerPermissionRequestChildProcessActor(
> +                                          WorkerPermissionChallenge* aChallenge)
> +    : mChallenge(aChallenge)
> +  {
> +    MOZ_ASSERT(aChallenge);

Assert that this is not the main process.

@@ +1202,5 @@
> +    MOZ_ASSERT(aFactory);
> +    mWorkerPrivate->AssertIsOnWorkerThread();
> +  }
> +
> +  NS_IMETHOD Run() override

Nit: Can you go through and make sure this and all the other methods have return values on their own line?

@@ +1215,5 @@
> +
> +  virtual bool Notify(JSContext* aCx, workers::Status aStatus) override
> +  {
> +    // We don't care about the notification. We just want to keep the
> +    // mWorkerPrivate alive.

Hm. We could just let remove the feature here and set some flags to make the prompt stuff be a no-op right? I don't think it's critical, but might want to file a followup so that we don't hold the worker alive for a long time.

@@ +1262,5 @@
> +    }
> +
> +    nsPIDOMWindow* window = wp->GetWindow();
> +    if (!window) {
> +      return true;

Maybe assert here that mPrincipalInfo is a system principal?

@@ +1273,5 @@
> +      return true;
> +    }
> +
> +    if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +

Nit: extra line

@@ +1274,5 @@
> +    }
> +
> +    if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +
> +       nsCOMPtr<Element> ownerElement =

Nit: indent is off here

@@ +1285,5 @@
> +        new WorkerPermissionRequest(this, ownerElement, principal);
> +
> +      PermissionRequestBase::PermissionValue permission;
> +      if (NS_WARN_IF(NS_FAILED(helper->PromptIfNeeded(&permission)))) {
> +      return true;

Nit: indent

@@ +1356,5 @@
> +    nsRefPtr<WorkerPermissionChallenge> challenge =
> +      new WorkerPermissionChallenge(workerPrivate, this, mFactory,
> +                                    aPrincipalInfo);
> +
> +    JSContext* cx = workerPrivate->GetJSContext();

MOZ_ASSERT(cx)
Attachment #8595649 - Flags: review?(bent.mozilla) → review+
> > +    nsPIDOMWindow* window = wp->GetWindow();
> > +    if (!window) {
> > +      return true;
> 
> Maybe assert here that mPrincipalInfo is a system principal?

What about SharedWorkers/ServiceWorkers/Sandboxes/etc ?
Flags: needinfo?(bent.mozilla)
Keywords: checkin-needed
(In reply to Andrea Marchesini (:baku) from comment #8)
> What about SharedWorkers/ServiceWorkers/Sandboxes/etc ?

Oh right. Boo. :(
Flags: needinfo?(bent.mozilla)
https://hg.mozilla.org/mozilla-central/rev/9641952c788f
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8601966 [details] [diff] [review]
242519.diff

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

I would really like to get this on branches. It was a dumb oversight that I left a MOZ_CRASH in here with bug 701634.
Attachment #8601966 - Flags: approval-mozilla-esr38?
Attachment #8601966 - Flags: approval-mozilla-beta?
Attachment #8601966 - Flags: approval-mozilla-aurora?
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #13)

> I would really like to get this on branches. It was a dumb oversight that I
> left a MOZ_CRASH in here with bug 701634.

Can you fill out the approval request form? It is fairly trivial but it helps release management to make sure all the information we need is there.  ESR and beta (38.0) are both already built at this point so to get this into the 38 release we would need to do a point release. What is the impact if we don't do that?
Flags: needinfo?(bent.mozilla)
(In reply to Liz Henry (:lizzard) from comment #14)
> Can you fill out the approval request form?

Dunno why but that isn't automatically popping up anymore. I figured someone got rid of it.

> we would need to do a point release. What is the impact if we don't do that?

Someone could write JS that could trigger a MOZ_CRASH.
Flags: needinfo?(bent.mozilla)
That's odd. The approval form automatic fill-in seems to still be working for me. Can you try it maybe in some other bug ? If that is broken I'd like to make sure we fix that. 

I would like to get this patch into 39. What are the risks for what might break, and what test coverage do we have that might help cover this?

Here are the questions from the form:

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: 
[Describe test coverage new/current, TreeHerder]: 
[Risks and why]: 
[String/UUID change made/needed]:
Flags: needinfo?(bent.mozilla)
(In reply to Liz Henry (:lizzard) from comment #16)
> That's odd. The approval form automatic fill-in seems to still be working
> for me. Can you try it maybe in some other bug ? If that is broken I'd like
> to make sure we fix that. 

Dunno, maybe it was broken on that day's nightly? Seems to work now.

> [Feature/regressing bug #]: 701634
> [User impact if declined]: Someone could write JS that could trigger a MOZ_CRASH.
> [Describe test coverage new/current, TreeHerder]: The patch includes tests
> [Risks and why]: This is new code so there is some (small) risk of regressions. On the other hand we don't want to MOZ_CRASH easily.
> [String/UUID change made/needed]: None
Flags: needinfo?(bent.mozilla)
Comment on attachment 8601966 [details] [diff] [review]
242519.diff

Approved for uplift to aurora. Patch includes tests. Let's see how it goes.
Attachment #8601966 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Right! We need bug 1149420 in aurora.
Depends on: 1149420
Flags: needinfo?(amarchesini)
Mind filling out an uplift request in that bug? :)
Flags: needinfo?(amarchesini)
Done. Let me know if I can do more.
Flags: needinfo?(amarchesini)
I'll wait till the patch from bug 1149420 lands before approving this for beta (now that 39 is in beta).
Comment on attachment 8601966 [details] [diff] [review]
242519.diff

Switching the approval flags around to reflect Monday's uplift.
Attachment #8601966 - Flags: approval-mozilla-aurora+ → approval-mozilla-release?
Comment on attachment 8601966 [details] [diff] [review]
242519.diff

Approved for uplift to beta (39) hoping to avoid a MOZ_CRASH
Attachment #8601966 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8601966 [details] [diff] [review]
242519.diff

It looks too big to be accept in the 38.0.5 release or ESR.
Please n-i if you disagree.
Attachment #8601966 - Flags: approval-mozilla-release?
Attachment #8601966 - Flags: approval-mozilla-release-
Attachment #8601966 - Flags: approval-mozilla-esr38?
Attachment #8601966 - Flags: approval-mozilla-esr38-
You need to log in before you can comment on or make changes to this bug.