Closed Bug 1400655 Opened 7 years ago Closed 7 years ago

json view does not work when service worker intercepts in e10s mode

Categories

(Core :: DOM: Service Workers, defect, P3)

57 Branch
Unspecified
All
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: damian.senn, Assigned: bkelly)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170915100121

Steps to reproduce:

Tried to view a JSON response in a tab, it usually pretty prints the JSON but got raw text.

Visit https://sarahtherad.com/wp-json/rad/home
Everything is fine

Visit https://sarahtherad.com/
Serviceworker is registered

Visit https://sarahtherad.com/wp-json/rad/home again


Actual results:

It shows the JSON in raw text. If I unregister the serviceworker everything is fine.


Expected results:

Show JSON View even if a serviceworker is present.
Component: Untriaged → Developer Tools: JSON Viewer
This only happens with e10s enabled.
Like bug 1397966, the problem is that getMIMETypeFromContent is not even called.
So there is not much that can be done from the JSON Viewer point of view, the fix should happen deep in netwerk code.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hi junior,

This seems a serviceworker related bug, could you help find the right person to handle it?
Flags: needinfo?(juhsu)
nsDocShell::DoChannelLoad sets the loadflag LOAD_CALL_CONTENT_SNIFFERS
but it is gone in the child process with InterceptedChannel

Change the component first
Component: Developer Tools: JSON Viewer → DOM: Service Workers
Flags: needinfo?(juhsu)
Product: Firefox → Core
Btw 56 looks good
(In reply to Junior[:junior] from comment #3)
> nsDocShell::DoChannelLoad sets the loadflag LOAD_CALL_CONTENT_SNIFFERS
> but it is gone in the child process with InterceptedChannel
> 
> Change the component first

Mmm... not sure which component I should set.

The load flag is set by docshell in content process, and the channel is intercepted.
Hence, no IPC to parent process for "this" channel, neither the sniffer.
Can someone run mozregression to see where this broke?
I can reproduce the problem back in Nightly 45.0 2015-11-17. And if previous builds work that's probably because https://sarahtherad.com/ crashes and the serviceworker is not registered (I believe that's bug 1224941).
Hmm, comment 4 suggests FF56 worked as expected.

Maybe this works in non-e10s mode but not e10s mode?
Yes, I already said this in comment 1.
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7f1714325cb3e40427e40031a2191fe20cdc3241&tochange=6aa1cc819058deaa13f85278e2f5970b8ced0998

Regressed by: Bug 1290481

:tt,
Your bunch of patch seems to cause the regression. Can you look into this?
Blocks: 1290481
Flags: needinfo?(ttung)
OS: Unspecified → All
Could you try to run mozregression with a fresh profile? I guess you ran it with the same profile and it was broken due to downgrade.

In theory, bug 1290481 changed the code related to DOM Cache and the QuotaManager. I'll remove the dependency for now because this issue is probably not related to bug 1290481. 

Please refer to comment Bug 1409115 Comment 12.
The patch P2, P6 in Bug 1290481 did the major upgrade to the DOM Cache the QuotaManager. Thus, QuotaManager and IndexedDB and ServiceWorkers will be broken if you use the same profile to test with/without patches in Bug 1290481.
No longer blocks: 1290481
Flags: needinfo?(ttung) → needinfo?(alice0775)
(In reply to Tom Tung [:tt] from comment #11)
> Could you try to run mozregression with a fresh profile? I guess you ran it
> with the same profile and it was broken due to downgrade.

Or you could try to upgrade to nightly with bug 1404344 [Note 1] and then switch to the build before bug 1290481 to see whether the issue still happens or not. Just like the suggestion in Bug 1409115 Comment 26.

Note 1: you still need to go to the website to trigger the patches in bug 1404344.
I tried to find regression with clean profile each time.

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9136548f3de862cf471ead56053fbf7298c350ca&tochange=4970f731aeba44e98d91ae54915a80dca39a5716

Regressed by:
	4970f731aeba	Andrea Marchesini — Bug 1393360 - nsMultiplexInputStream should not QI each stream when checking if seekable, cloneable or IPC serializable, r=smaug
	0d9988ed1c6e	Andrea Marchesini — Bug 1392358 - Introduce XHR.sendInputStream(nsIInputStream) chrome-only, r=smaug
	586a6514e046	Andrea Marchesini — Bug 1392358 - Remove XHR.send(nsIInputStream) and unify XHR.send(params) as the spec says, r=smaug


:baku
Your patch seems to cause the regression. Can you look into this?
Flags: needinfo?(alice0775) → needinfo?(amarchesini)
Alice, I'm sorry, but I probably should have removed the regressionwindow-wanted after comment 7.  I can also reproduce this in FF56.  It seems this has always been broken in e10s mode.  I suspect mozregression was bouncing between e10s and non-e10s based on the experiment cohort stuff?

Also, this now fails in FF58 non-e10s since I changed how we do intercepted there in bug 1391693.

I think the solution here is that our intercept response pump code needs to perform this same code from nsHttpChannel:

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#1610

I'll take this since I was just touching this code.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(amarchesini)
Summary: json view serviceworker → json view does not work when service worker intercepts in e10s mode
This patch fixes the problem for me locally in both e10s and non-e10s.  Let me see if I can write a test.
While you are at it, maybe you can fix bug 1397966 (sniffers are not called when there is no content)?
(In reply to Oriol Brufau [:Oriol] from comment #16)
> While you are at it, maybe you can fix bug 1397966 (sniffers are not called
> when there is no content)?

I'd rather leave that to a separate bug since it involves touching non-service-worker code.  The issue there is that nsHttpChannel does not do:

https://searchfox.org/mozilla-central/source/netwerk/base/nsBaseChannel.cpp#848

Its unclear to me how the necko team would like to fix that, though.  There is a desire to make nsHttpChannel use nsBaseChannel, for example.
Jan, this test fails with current mozilla-central, but passes with the P1 patch.  Its goal is to check that we properly trigger the jsonview even when the response is synthesized by a service worker.
Attachment #8920679 - Flags: review?(odvarko)
Comment on attachment 8920603 [details] [diff] [review]
P1 Respect LOAD_CALL_CONTENT_SNIFFERS when a channel is intercepted by a ServiceWorker. r=valentin

Valentin, this patch allows us to correctly handle LOAD_CALL_CONTENT_SNIFFERS on intercepted channels.  I moved some of the code in nsHttpChannel into HttpBaseChannel to avoid further code duplication.
Attachment #8920603 - Flags: review?(valentin.gosu)
Sorry, the previous patch was from the wrong tree.  This one is correct.
Attachment #8920679 - Attachment is obsolete: true
Attachment #8920679 - Flags: review?(odvarko)
Attachment #8920684 - Flags: review?(odvarko)
Attachment #8920603 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8920684 [details] [diff] [review]
P2 Add a mochitest to verify the json viewer is shown for json intercepted by a service worker. r=honza

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

Thanks, this look good to me!
Attachment #8920684 - Flags: review?(odvarko) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84cb594525ad
P1 Respect LOAD_CALL_CONTENT_SNIFFERS when a channel is intercepted by a ServiceWorker. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/574b5b042c3c
P2 Add a mochitest to verify the json viewer is shown for json intercepted by a service worker. r=jryans
Backed out for eslint failure in devtools/client/jsonview/test/browser_jsonview_serviceworker.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/de6b2b70a3003daa8f82812568ec3e512a14c3d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/beff4e58e183d639063b1334452add368f82d269

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=574b5b042c3c4964b1837fcbcb23557c29be51e9&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139638094&repo=mozilla-inbound

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/jsonview/test/browser_jsonview_serviceworker.js:12:24 | Missing space before function parentheses. (space-before-function-paren)
...
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/jsonview/test/passthrough-sw.js:1:1 | Use the global form of 'use strict'. (strict)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/jsonview/test/passthrough-sw.js:1:18 | Strings must use doublequote. (quotes)
Flags: needinfo?(bkelly)
Fix eslint issues.  This passes eslint locally.
Attachment #8920684 - Attachment is obsolete: true
Flags: needinfo?(bkelly)
Attachment #8922360 - Flags: review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6d464fd41c6
P1 Respect LOAD_CALL_CONTENT_SNIFFERS when a channel is intercepted by a ServiceWorker. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/39fb91da0ea7
P2 Add a mochitest to verify the json viewer is shown for json intercepted by a service worker. r=jryans
Priority: P4 → P3
https://hg.mozilla.org/mozilla-central/rev/b6d464fd41c6
https://hg.mozilla.org/mozilla-central/rev/39fb91da0ea7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: