Bug 182070 - Offlined content does not work for apps on home screen
Summary: Offlined content does not work for apps on home screen
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: Safari 11
Hardware: iPhone / iPad iOS 11
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 182375
  Show dependency treegraph
 
Reported: 2018-01-24 15:39 PST by Thomas Steiner
Modified: 2018-03-20 15:50 PDT (History)
13 users (show)

See Also:


Attachments
Screenshot of failing attempt to load an offlined app from home screen in airplane mode (137.73 KB, image/png)
2018-01-24 15:39 PST, Thomas Steiner
no flags Details
WIP Patch (2.66 KB, patch)
2018-01-26 10:30 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (8.54 KB, patch)
2018-01-26 10:56 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (17.33 KB, patch)
2018-01-26 12:42 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (17.33 KB, patch)
2018-01-26 13:31 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Steiner 2018-01-24 15:39:26 PST
Created attachment 332208 [details]
Screenshot of failing attempt to load an offlined app from home screen in airplane mode

Steps to reproduce:

1) Go to https://airhorner.com/, reload twice to make sure the Service Worker is installed and active. 
2) Turn on airplane mode, then reload. 
3) Works as intended. 
4) Turn off airplane mode again. 
5) Add the app to the home screen. 
6) Turn on airplane mode again. 
7) Try launching the app from the home screen.
8) This fails. 

Even if after 5) you launch the app a couple of times from the home screen while online and only then launch it again in airplane mode, it doesn’t work.

Tested on: Mozilla/5.0 (iPhone; CPU iPhone OS 11_3 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/11.0 Mobile/15E148 Safari/604.1
Comment 1 Radar WebKit Bug Importer 2018-01-24 15:50:46 PST
<rdar://problem/36843906>
Comment 2 Chris Dumez 2018-01-26 10:30:50 PST
Created attachment 332383 [details]
WIP Patch
Comment 3 Chris Dumez 2018-01-26 10:56:35 PST
Created attachment 332387 [details]
WIP Patch
Comment 4 Chris Dumez 2018-01-26 11:01:59 PST
Will write an API test shortly.
Comment 5 Chris Dumez 2018-01-26 12:42:23 PST
Created attachment 332403 [details]
Patch
Comment 6 EWS Watchlist 2018-01-26 12:43:28 PST
Attachment 332403 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:238:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:242:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:242:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:243:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:246:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:248:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:249:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:259:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:270:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:273:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:280:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 11 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Simon Fraser (smfr) 2018-01-26 13:30:17 PST
Comment on attachment 332403 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332403&action=review

> Source/WebCore/ChangeLog:13
> +        We know initialize the registrations' active worker as soon as we load

know -> now
Comment 8 Chris Dumez 2018-01-26 13:31:49 PST
Created attachment 332411 [details]
Patch
Comment 9 EWS Watchlist 2018-01-26 13:33:26 PST
Attachment 332411 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:238:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:242:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:242:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:243:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:246:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:248:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:249:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:259:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:270:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:273:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:280:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 11 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 youenn fablet 2018-01-26 13:39:39 PST
Comment on attachment 332411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332411&action=review

> Source/WebCore/workers/service/server/SWServer.cpp:142
> +    auto registrationPtr = registration.get();

auto& registrationRef?
Comment 11 Chris Dumez 2018-01-26 13:40:47 PST
Comment on attachment 332411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332411&action=review

>> Source/WebCore/workers/service/server/SWServer.cpp:142
>> +    auto registrationPtr = registration.get();
> 
> auto& registrationRef?

I think we use this Ptr pattern a lot.
Comment 12 WebKit Commit Bot 2018-01-26 14:11:10 PST
Comment on attachment 332411 [details]
Patch

Clearing flags on attachment: 332411

Committed r227696: <https://trac.webkit.org/changeset/227696>
Comment 13 WebKit Commit Bot 2018-01-26 14:11:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Thomas Steiner 2018-03-20 05:46:29 PDT
I am still seeing this issue on Beta 6 (Build 15E5216a). Any info if the landed patches make it into any of the upcoming Betas?
Comment 15 Chris Dumez 2018-03-20 09:03:28 PDT
(In reply to Thomas Steiner from comment #14)
> I am still seeing this issue on Beta 6 (Build 15E5216a). Any info if the
> landed patches make it into any of the upcoming Betas?

The fix for this bug went into beta 3 iirc. I have tested airhorner many times from home screen and it was working.
Comment 16 Chris Dumez 2018-03-20 09:07:10 PDT
(In reply to Chris Dumez from comment #15)
> (In reply to Thomas Steiner from comment #14)
> > I am still seeing this issue on Beta 6 (Build 15E5216a). Any info if the
> > landed patches make it into any of the upcoming Betas?
> 
> The fix for this bug went into beta 3 iirc. I have tested airhorner many
> times from home screen and it was working.

I have just tried again on the latest beta the exact steps described in this bug's original description. It works for me.

I do see a popup complaining about airplane mode being on once in a while. But tapping OK, I can see and use the app offline. I think we investigated this and the popup is showing due to the app doing analytics and trying to go to the network.
Comment 17 Thomas Steiner 2018-03-20 09:10:16 PDT
I can confirm this works for Airhorner.

However, when I test with trivago.com, it doesn't. Same repro steps as in the original report, just with https://www.trivago.com as the URL.
Comment 18 Ryosuke Niwa 2018-03-20 15:50:28 PDT
(In reply to Thomas Steiner from comment #17)
> I can confirm this works for Airhorner.
> 
> However, when I test with trivago.com, it doesn't. Same repro steps as in
> the original report, just with https://www.trivago.com as the URL.

Tracking this issue in https://bugs.webkit.org/show_bug.cgi?id=183800.