Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove navigation support for multipart/x-mixed-replace resources #1353

Closed
wants to merge 1 commit into from

Conversation

annevk
Copy link
Member

@annevk annevk commented May 31, 2016

As per https://bugzilla.mozilla.org/show_bug.cgi?id=1276918 this is not
widely implemented (except for images). As per
https://bugs.chromium.org/p/chromium/issues/detail?id=249132 the
complexity of this feature is a security a concern. Coupled with it
being rarely used by all accounts there’s ample reason to remove it
from the HTML Standard.

As per https://bugzilla.mozilla.org/show_bug.cgi?id=1276918 this is not
widely implemented (except for images). As per
https://bugs.chromium.org/p/chromium/issues/detail?id=249132 the
complexity of this feature is a security a concern. Coupled with it
being rarely used by all accounts there’s ample reason to remove it
from the HTML Standard.
@annevk annevk mentioned this pull request May 31, 2016
6 tasks
@annevk
Copy link
Member Author

annevk commented May 31, 2016

Note that this conflicts with #1346. I've no preference on what we do first though.

@domenic
Copy link
Member

domenic commented May 31, 2016

I'd like to hear a bit more from implementers. Right now from reading those bugs I gather the matrix is:

  • Blink: supports for images
  • Edge: does not support at all
  • Gecko: supports for all resources
  • WebKit: supports "half-baked".

Although I agree that this clearly doesn't meet the bar as-is, it's possible one outcome here would be that we leave it in for images only. @bzbarsky has already commented in the Gecko bug and seems in favor of leaving it in. @cdumez, @DigiTec, as our go-to WebKit and Edge people, any thoughts on this issue? @avidrissman, I know you and one other engineer whose name I forgot were working on navigate a lot in Blink; have you run into this code?

@avidrissman
Copy link

I'm new enough on nav that I haven't yet run into this. The other nav expert is @csreis. Charlie, WDYT?

@bzbarsky
Copy link
Contributor

I don't know that I'm in favor of leaving it in per se; I just think that trying to remove it will break lots of things, so is probably not viable. :(

@annevk
Copy link
Member Author

annevk commented May 31, 2016

@domenic this is specifically only about navigation where only Gecko has actual support.

I need to make sure that it results in a network error though, I think, rather than just an unknown MIME type.

@annevk
Copy link
Member Author

annevk commented May 31, 2016

Alternatively we render it as HTML which Edge appears to do, but seems rife with potential XSS.

@domenic
Copy link
Member

domenic commented May 31, 2016

You can navigate to images.

@cdumez
Copy link

cdumez commented May 31, 2016

Relevant WebKit discussion:
https://lists.webkit.org/pipermail/webkit-dev/2015-April/027379.html

We decided to keep support for multipart/x-mixed-replace main resources (which I believe applies here in the context of navigation) due to certain WebCam pages that relied on it (having a MJPEG main resource).

@annevk
Copy link
Member Author

annevk commented May 31, 2016

Yeah, supporting images seems good. That would still remove most of the complexity of this recursively invoking navigate though since we'd reuse the image codepath. At least, as far as I can tell.

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label May 31, 2016
@csreis
Copy link

csreis commented May 31, 2016

I'm not familiar with this area either. Looking at https://crrev.com/152363 where it was removed, I'd suggest asking japhet@chromium.org (who knows more about DocumentLoader). Not sure what his handle is here, so I'll send him a link.

@natechapin
Copy link

Blink removed support for general multipart/x-mixed-replace navigations in https://src.chromium.org/viewvc/blink?view=revision&revision=152363 and added back in narrow support for images-as-main-resource in https://chromium.googlesource.com/chromium/src/+/3b48d99c334c370b39f041ef3a6f743faa61b426. I think restrict to images is a good tradeoff, as MJPEG seems to be the case users have cared about.

@annevk
Copy link
Member Author

annevk commented Jun 1, 2016

Okay, so as-is this is broken. I guess I'll need to write some more tests unless anyone knows of existing test coverage?

@annevk
Copy link
Member Author

annevk commented Jul 4, 2016

8b630f5 refined things a bit here, but I'm going to close this for now as it looks like this requires much more research that doesn't seem urgent relative to other issues.

@annevk annevk closed this Jul 4, 2016
@domenic domenic deleted the remove-multipart/x-mixed-replace branch July 12, 2016 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet Pull request must not be merged per rationale in comment topic: navigation
Development

Successfully merging this pull request may close these issues.

7 participants