[non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: noemi, Assigned: jaoo)

Tracking

(Depends on: 1 bug)

Trunk
FxOS-S3 (24Jul)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox42 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Hi,

scenario checked with today's (6/29) master build in Nightly Debug (eaf4f9b45117 revision)

STRs:
1- register a sw
2- open about:sw in another tab (the sw is properly shown)
3- change the sw
3- click on the "update" button within about:sw tab

Actual result:
nothing happens

Expected result:
The update process is properly initiated and completed

Note: you can run step 1 and then step 2 or step 2 and then step 1 the result is exactly the same
(Reporter)

Updated

3 years ago
Summary: [non-e10s] Update process within about:serviceworkers doesn't work in non-e10s mode → [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode
tracking-e10s: --- → -
(Assignee)

Comment 1

3 years ago
I'm suffering this in the tests I'm adding on bug 1171917. If I'm not wrong this is because about:sw is chrome process and when propagating the update the ServiceWorkerManagerService object doesn't notify the update because of the check at [1]. Andrea, is that correct? Thanks.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManagerService.cpp#170
Flags: needinfo?(amarchesini)
(Assignee)

Updated

3 years ago
See Also: → bug 1171917
> https://mxr.mozilla.org/mozilla-central/source/dom/workers/
> ServiceWorkerManagerService.cpp#170

This check means that we don't dispatch a notification for update at the parent who triggered it.
Why do you think this is wrong?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 3

3 years ago
(In reply to Andrea Marchesini (:baku) from comment #2)
> > https://mxr.mozilla.org/mozilla-central/source/dom/workers/
> > ServiceWorkerManagerService.cpp#170
> 
> This check means that we don't dispatch a notification for update at the
> parent who triggered it.
> Why do you think this is wrong?

Because the update is not notified and the service worker is never updated.
(Assignee)

Updated

3 years ago
Assignee: nobody → jaoo
Blocks: 1153312, 1171917
Target Milestone: --- → FxOS-S2 (10Jul)
(Assignee)

Comment 4

3 years ago
Created attachment 8628809 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku

Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku
Attachment #8628809 - Flags: review?(amarchesini)
(Assignee)

Comment 5

3 years ago
Created attachment 8628810 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku

Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku
Attachment #8628810 - Flags: review?(amarchesini)
Attachment #8628810 - Flags: review?(amarchesini) → review+
Comment on attachment 8628810 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku

https://reviewboard.mozilla.org/r/12477/#review10923

Ship It!

::: dom/workers/test/serviceworkers/test_aboutserviceworkers.html:45
(Diff revision 1)
> +    

extra spaces
Attachment #8628809 - Flags: review?(amarchesini)
Comment on attachment 8628809 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku

https://reviewboard.mozilla.org/r/12475/#review10921

::: dom/workers/ServiceWorkerManager.cpp:4714
(Diff revision 1)
> +  }

I think you just need to do:

SoftUpdate(aOriginAttributes, NS_ConvertUTF16toUTF8(aScope));
mActor->SendPropagateSoftUpdate(...);
(Assignee)

Comment 8

3 years ago
Comment on attachment 8628809 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku

Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku
Attachment #8628809 - Flags: review?(amarchesini)
(Assignee)

Updated

3 years ago
Attachment #8628810 - Flags: review+ → review?(amarchesini)
(Assignee)

Comment 9

3 years ago
Comment on attachment 8628810 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku

Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku
Comment on attachment 8628810 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku

Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku
Attachment #8628810 - Flags: review?(amarchesini)
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
Attachment #8628810 - Flags: review+
Comment on attachment 8628810 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku

https://reviewboard.mozilla.org/r/12477/#review11065

Ship It!
Comment on attachment 8628809 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku

Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku
Comment on attachment 8628810 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku

Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku
Attachment #8628810 - Flags: review+
Comment on attachment 8628809 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku

https://reviewboard.mozilla.org/r/12475/#review11405

Ship It!
Attachment #8628809 - Flags: review?(amarchesini) → review+
(In reply to Pulsebot from comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2db7f8ad8c8f
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ab0df10f86e2

The patches landed here will be backed out as I've just noticed an issue. The tests will pass but a new issue will be added. :tomcat will back them out. Sorry for the noise.
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #17)
> (In reply to Pulsebot from comment #16)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/2db7f8ad8c8f
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/ab0df10f86e2
> 
> The patches landed here will be backed out as I've just noticed an issue.
> The tests will pass but a new issue will be added. :tomcat will back them
> out. Sorry for the noise.

np, backed out now from mozilla-inbound
Comment on attachment 8628809 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku

Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku
Attachment #8628809 - Flags: review+ → review?(amarchesini)
Comment on attachment 8628810 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku

Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku
https://reviewboard.mozilla.org/r/12475/#review11497

::: dom/workers/ServiceWorkerManagerService.cpp:169
(Diff revision 4)
>  

Andrea, I've changed the check from the child to the parent. It seems the right approach (IIRC you said something similar during our IRC discussion yesterday). The previous version of it was backed out because I wanted to double check whether everything was fine). This version fixes the issue reported in this bug and doesn't add anything wrong ;).
Hey Jaoo, seems one test also problems like https://treeherder.mozilla.org/logviewer.html#?job_id=11519983&repo=mozilla-inbound - that is also in the try run here
Comment on attachment 8628809 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku

Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku
(Assignee)

Updated

3 years ago
Attachment #8628810 - Flags: review?(amarchesini)
Comment on attachment 8628810 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku

Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku
(In reply to Carsten Book [:Tomcat] from comment #23)
> Hey Jaoo, seems one test also problems like
> https://treeherder.mozilla.org/logviewer.html#?job_id=11519983&repo=mozilla-
> inbound - that is also in the try run here

Thanks, there was a race condition I should be fixed now. Waiting a try to verify that.
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #26)
> (In reply to Carsten Book [:Tomcat] from comment #23)
> > Hey Jaoo, seems one test also problems like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=11519983&repo=mozilla-
> > inbound - that is also in the try run here
> 
> Thanks, there was a race condition I should be fixed now. Waiting a try to
> verify that.

Green! https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4d398733383
(Reporter)

Updated

3 years ago
Target Milestone: FxOS-S2 (10Jul) → FxOS-S3 (24Jul)
Comment on attachment 8628809 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku

Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku
Comment on attachment 8628810 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku

Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku
Attachment #8628809 - Flags: review?(amarchesini) → review+
Comment on attachment 8628809 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. r=baku

https://reviewboard.mozilla.org/r/12475/#review11575

Ship It!

::: dom/workers/ServiceWorkerManagerService.h:56
(Diff revision 5)
> +  uint32_t AgentsCount();

const

::: dom/workers/ServiceWorkerManagerService.cpp:174
(Diff revision 6)
> -  } else {
> +  data->mParentFound = data->mParentFound || parent->ID() == data->mParentID;

if (parent->ID() == data->mParentID) {
  data->mParentFound = true;
}
Comment on attachment 8628810 [details]
MozReview Request: Bug 1178233 - [non-e10s] The update process doesn't work within about:serviceworkers in non-e10s mode. Test. r=baku

https://reviewboard.mozilla.org/r/12477/#review11717

Ship It!
Attachment #8628810 - Flags: review?(amarchesini) → review+
(Reporter)

Comment 33

3 years ago
Hi,


just checked scenario in comment 0 with m-i build in Nightly Debug (7b876dcebaeb revision) and the update process is properly launched. Once this patch lands in m-c the update process will be checked both in desktop and b2g. Thanks!
https://hg.mozilla.org/mozilla-central/rev/c05f3df663bf
https://hg.mozilla.org/mozilla-central/rev/7b876dcebaeb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
(Reporter)

Comment 35

3 years ago
Hi,

just checked with m-c Nightly Debug (c95ebeebbc5d revision) and the update process is launched under a non-e10s scenario although the activate event is fired even when the sw is controlling the client, which shouldn't. It seems we should wait for SWM refactor (Bug 1182117)

Updated

3 years ago
Depends on: 1200906

Updated

3 years ago
Depends on: 1200912
You need to log in before you can comment on or make changes to this bug.