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)
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: damian.senn, Assigned: bkelly)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files, 2 obsolete files)
4.93 KB,
patch
|
valentin
:
review+
|
Details | Diff | Splinter Review |
4.72 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•7 years ago
|
Component: Untriaged → Developer Tools: JSON Viewer
Comment 1•7 years ago
|
||
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
Updated•7 years ago
|
status-firefox57:
--- → fix-optional
Priority: -- → P4
Comment 2•7 years ago
|
||
Hi junior, This seems a serviceworker related bug, could you help find the right person to handle it?
Flags: needinfo?(juhsu)
Comment 3•7 years ago
|
||
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
Comment 5•7 years ago
|
||
(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.
Assignee | ||
Comment 6•7 years ago
|
||
Can someone run mozregression to see where this broke?
Blocks: dt-service-worker, ServiceWorkers-compat
Keywords: regression,
regressionwindow-wanted
Comment 7•7 years ago
|
||
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).
Assignee | ||
Comment 8•7 years ago
|
||
Hmm, comment 4 suggests FF56 worked as expected. Maybe this works in non-e10s mode but not e10s mode?
Comment 10•7 years ago
|
||
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?
Comment 11•7 years ago
|
||
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)
Comment 12•7 years ago
|
||
(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.
Comment 13•7 years ago
|
||
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)
Assignee | ||
Comment 14•7 years ago
|
||
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)
Keywords: regression,
regressionwindow-wanted
Summary: json view serviceworker → json view does not work when service worker intercepts in e10s mode
Assignee | ||
Comment 15•7 years ago
|
||
This patch fixes the problem for me locally in both e10s and non-e10s. Let me see if I can write a test.
Comment 16•7 years ago
|
||
While you are at it, maybe you can fix bug 1397966 (sniffers are not called when there is no content)?
Assignee | ||
Comment 17•7 years ago
|
||
(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.
Assignee | ||
Comment 18•7 years ago
|
||
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)
Assignee | ||
Comment 19•7 years ago
|
||
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)
Assignee | ||
Comment 20•7 years ago
|
||
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)
Assignee | ||
Comment 21•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=55eaea12867a5739f5ef274ba5807ce17300b7a5
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+
Comment 23•7 years ago
|
||
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
Comment 24•7 years ago
|
||
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)
Assignee | ||
Comment 25•7 years ago
|
||
Fix eslint issues. This passes eslint locally.
Attachment #8920684 -
Attachment is obsolete: true
Flags: needinfo?(bkelly)
Attachment #8922360 -
Flags: review+
Comment 26•7 years ago
|
||
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
Updated•7 years ago
|
Priority: P4 → P3
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b6d464fd41c6 https://hg.mozilla.org/mozilla-central/rev/39fb91da0ea7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•