1285955 - Unify some code in nsHostObjectProtocolHandler

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrea Marchesini [:baku]]

Attachment: blob_multi_e10s_a.patch

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

So this is changing the behavior. Can you please explain the query '?' handling here?

It is currently unclear to me why query needs to be handled at all. Is that coming from some spec?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8769689 [details] [diff] [review]
blob_multi_e10s_a.patch

Please explain and ask review again.

Comment 3

[Andrea Marchesini [:baku]]

GetDataInfo does some URL splitting ('#' and '?'). RemoveDataEntry does something similar. I just wanted to merge these 2 operations in 1 single method.

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8769689 [details] [diff] [review]
blob_multi_e10s_a.patch

The patch moved code mainly and it unifies how GetDataInfo and RemoveDataEntry parse strings.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8769689 [details] [diff] [review]
blob_multi_e10s_a.patch

RemoveDataEntry does nothing with '?' currently.
Is that a bug in current RemoveDataEntry?
Or do we have bug in current GetDataInfo callers that they care about '?' or what?
Or do we not need to care about '?' at all in RemoveDataEntry? If so, why? (and why we'd need to care about '#' there still)

Comment 6

[Andrea Marchesini [:baku]]

> RemoveDataEntry does nothing with '?' currently.
> Is that a bug in current RemoveDataEntry?

RemoveDataEntry and GetDataInfo are out of sync. Before we had the support only for '#'. Then in bug 944616 I introduced the support for '?' but I didn't change RemoveDataEntry. This means that you can open: 'blob:something?123' but you cannot remove the same URL entry.

> Or do we not need to care about '?' at all in RemoveDataEntry? If so, why?
> (and why we'd need to care about '#' there still)

'#' has been introduced by bug 920877. I think RemoveDataEntry should be at least in sync with the rest of the code.

But but but, from a spec point of view, removeObjectURL should remove the exact blobURL. So any '#' '?' should be consider as part of the blobURL. I think we should follow the spec. I'll submit a new patch.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: blob_multi_e10s.patch

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

So we need GetDataInfo to behave as it behaves so that we can get principal from the string-url, even if it contains + or #. ok.

(The other use cases for GetDataInfo are less clear to me, but at least we aren't changing any that behavior.)


Chrome doesn't seem to throw like Gecko with XHR, but I was testing
var  blob = new Blob(['hello world']); var url = URL.createObjectURL(blob); var part = "#part"; URL.revokeObjectURL(url + part);   window.open(url + part);
and
var  blob = new Blob(['hello world']); var url = URL.createObjectURL(blob); var part = "?part"; URL.revokeObjectURL(url + part);   window.open(url + part);

It seems to not revoke in #part case, but it does in ?part case.
So opposite of our current behavior.

Not quite sure what Edge is doing. I was told for ?part case it gives "DOM7001: Invalid argument 'url'. Failed to revoke Blob URL:"

My spec reading agrees with yours, and patch gives that behavior.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I think it would be good to do some more testing on Edge at least.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8769875 [details] [diff] [review]
blob_multi_e10s.patch

r+ since this is rather sane, and follows the spec.

Or, if we want different setup, we need a spec bug.

This is a bit regression risky, I think.

Comment 11

[Andrea Marchesini [:baku]]

> This is a bit regression risky, I think.

Chrome also does the same as this patch does.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

It does? Even with '?'.
Maybe I didn't find a good test for it.


Worth to push to try before pushing to m-i.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2641c0eeba
URL.revokeObjectURL should not remove '?' or '#' from the input, r=smaug

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f2641c0eeba