Closed Bug 1191647 Opened 6 years ago Closed 6 years ago

Listen to clear-origin-data in ServiceWorkerManager.cpp

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
FxOS-S6 (04Sep)
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: allstars.chh, Assigned: dimi)

References

Details

Attachments

(1 file, 3 obsolete files)

We should replace webapps-clear-data with clear-origin-data.
Assignee: nobody → dlee
Attached patch Patch v1 (obsolete) — Splinter Review
This patch change "webapps-clear-data" to "clear-origin-data"
Attachment #8645574 - Flags: feedback?(allstars.chh)
Comment on attachment 8645574 [details] [diff] [review]
Patch v1

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +78,1 @@
>  using namespace mozilla;

#include "mozIApplicationClearPrivateDataParams.h" should be removed.

@@ +4505,1 @@
>  UnregisterIfMatchesClearPrivateDataParams(const nsACString& aScope,

This could be renamed to a shorter name,

@@ +4509,5 @@
>    UnregisterIfMatchesUserData* data =
>      static_cast<UnregisterIfMatchesUserData*>(aPtr);
>  
>    if (data->mUserData) {
> +    OriginAttributes *params = static_cast<OriginAttributes*>(data->mUserData);

OriginAttributes* params

@@ +4514,5 @@
>      MOZ_ASSERT(params);
>      MOZ_ASSERT(aReg->mPrincipal);
>  
> +    uint32_t appId = params->mAppId;
> +    bool browserOnly = params->mInBrowser;

I didn't understand how service-worker work on this part, 
but in general we should use 
OriginAttributes A == OriginAttributes B,
or using nsIPrincipal.equals/subsumes.
Attachment #8645574 - Flags: feedback?(allstars.chh)
Target Milestone: --- → FxOS-S5 (21Aug)
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8645574 - Attachment is obsolete: true
Attachment #8646211 - Flags: feedback?(allstars.chh)
Attachment #8646211 - Flags: feedback?(allstars.chh) → feedback+
Attachment #8646211 - Flags: review?(bkelly)
Comment on attachment 8646211 [details] [diff] [review]
Patch v2

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

r=me with comments addressed.  Thanks.

::: dom/workers/ServiceWorkerManager.cpp
@@ +4507,3 @@
>  {
>    UnregisterIfMatchesUserData* data =
>      static_cast<UnregisterIfMatchesUserData*>(aPtr);

A bit unrelated to your patch, but can you add MOZ_ASSERT(data) here?

@@ +4513,1 @@
>      MOZ_ASSERT(params);

Please add a MOZ_ASSERT(aReg) here.  And this assert can go because its inside the if(data->mUserData).

@@ +4840,3 @@
>      MOZ_ASSERT(XRE_IsParentProcess());
> +    OriginAttributes attrs;
> +    attrs.Init(nsAutoString(aData));

Please check this return value.  If we think this can never fail then MOZ_ALWAYS_TRUE() should be fine.
Attachment #8646211 - Flags: review?(bkelly) → review+
Attached patch Patch V3 (obsolete) — Splinter Review
Address ben's comment
Attachment #8646211 - Attachment is obsolete: true
Attachment #8646728 - Flags: review+
There is error when running try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eee4a87e143

I am figuring out the root cause.
Attached patch Patch v4Splinter Review
- Rebase to latest code
- Fix try error by using MOZ_ALWAYS_TRUE
Attachment #8646728 - Attachment is obsolete: true
Attachment #8649706 - Flags: review+
Keywords: checkin-needed
Looks like it was a different bug in that checkin-needed push that caused the bustage. This can probably reland as-is.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/60b0b9b99e16
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: FxOS-S5 (21Aug) → FxOS-S6 (04Sep)
Flags: needinfo?(dlee)
Depends on: 1233136
You need to log in before you can comment on or make changes to this bug.