Closed Bug 1169044 Opened 9 years ago Closed 9 years ago

Fetch uses wrong referrer

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: franziskus, Assigned: nsm)

References

Details

Attachments

(6 files, 1 obsolete file)

MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey
39 bytes, text/x-review-board-request
khuey
: review+
Details
MozReview Request: Bug 1169044 - Patch 2 - Split URLSearchParams parsing logic into non-CCed URLParams. r?baku
40 bytes, text/x-review-board-request
baku
: review+
Details
MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan
40 bytes, text/x-review-board-request
bholley
: review+
Details
MozReview Request: Bug 1169044 - Patch 4 - Set referrer on intercepted request. r=jdm
40 bytes, text/x-review-board-request
Details
Patch 2 bugzilla
14.47 KB, patch
baku
: review+
Details | Diff | Splinter Review
Patch 6 - wpt expectations update
110.13 KB, patch
jgraham
: review+
Details | Diff | Splinter Review 
Firefox seems to send the referrer of the parent page when using fetch [1] instead of the current url.
This breaks most W3C referrer policy tests using fetch.


[1] https://fetch.spec.whatwg.org/
Flags: needinfo?(nsm.nikhil)
Do you have a small test case?
Flags: needinfo?(nsm.nikhil) → needinfo?(franziskuskiefer)
Created a very simple test case:

click the link: https://www.franziskuskiefer.de/data/fetchTest.html
expected: the referer for fetch is something with https://www.franziskuskiefer.de
actual: the referer for fetch is something with https://bugzilla.mozilla.org

copy&paste the link: https://www.franziskuskiefer.de/data/fetchTest.html
expected: the referer for fetch is something with https://www.franziskuskiefer.de
actual: fetch has no referer
Flags: needinfo?(franziskuskiefer)
Seems we don't quite implement all the steps right in the Determine Requests Referrer algorithm:

  https://w3c.github.io/webappsec/specs/referrer-policy/#determine-requests-referrer

So this seems wrong:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Fetch.cpp#422

And I don't see where we use an iframe's parent document.  I think the null principal handling might deal with the sandboxed iframe case.
https://reviewboard.mozilla.org/r/9599/#review8371

::: dom/fetch/Fetch.cpp:448
(Diff revision 1)
> +      aRequest->SetReferrer(NS_ConvertUTF8toUTF16(referrer));

Uses logic similar to XHR to deal with pushState. The "determine referrer" algorithm talks about srcdoc attributes on iframes. Gecko deals with this internally so that the nsIDocument created from the srcdoc content has the correct referrer. I checked in jsfiddle.
Comment on attachment 8612572 [details]
MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey

Did all that complexity get copy/pasted from somewhere?  If so, can we factor it out into a helper function?
(In reply to Not doing reviews right now from comment #6)
> Comment on attachment 8612572 [details]
> MozReview Request: Bug 1169044 - Set correct referrer on load from window
> global. r=bkelly
> 
> Did all that complexity get copy/pasted from somewhere?  If so, can we
> factor it out into a helper function?

XHR. I could probably refactor a few lines from 'get the principal' and 'get the doc' onwards into a function that accepts a principal and a doc and returns the referrer. The way XHR obtains its doc from the global is different though (I'm not sure why it does a combination of GetContextForEventHandlers + GetDocumentFromScriptContext when it has access to the window global.) If I can change that I can do a more significant refactoring.
Hmm.  smaug might know why xhr is doing what it's doing...
I would say use of GetDocumentFromScriptContext is a leftover. And since XHR used to have mScriptContext, something was needed to get nsIScriptContext and GetContextForEventHandlers was the easiest way.
GetContextForEventHandlers() does do innerwindow correctness check, so one can't open or send XHR 
to http(s) url if it is not from the current inner window.
We should probably tighten that up and do similar checks also for other networking stuff for consistency.

But if you want to factor out referrer handling, just add some 
CheckInnerWindowCorrectness(); to XHR (to keep the existing behavior) and otherwise use GetOwner()->GetExtantDoc() or something?
Flags: needinfo?(bugs)
Though, out setup for security checks in general has changed quite a bit, that perhaps we should remove
most of the inner window checks, at least the ones from Bug 403168.
Loadgroup usage can still be an issue though. (using loadgroup which is now used for some window)
but if we do any generic changes to loadgroup handling or innerwindow checks, those clearly don't belong to this bug.
Comment on attachment 8612572 [details]
MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey

https://reviewboard.mozilla.org/r/9599/#review8397

::: dom/fetch/Fetch.cpp:448
(Diff revision 1)
> +      aRequest->SetReferrer(NS_ConvertUTF8toUTF16(referrer));

Can we unify this logic in a single shared method?  Maybe something like nsContentUtils::GetReferrerURI(aGlobal, aDoc)?

Also, can you add a comment that iframe srcdoc referrer handling is done transparently by the document?
Attachment #8612572 - Flags: review?(bkelly)
Comment on attachment 8612572 [details]
MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey

https://reviewboard.mozilla.org/r/9599/#review8401

Overall looks reasonable.  It would be nice to factory out the common code.

Also, please add a test that catches the main failure modes from this bug.

Thanks!
Attachment #8612572 - Flags: review+
Comment on attachment 8612572 [details]
MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey

Bug 1169044 - Set correct referrer on load from window global. r?bkelly,smaug
Attachment #8612572 - Attachment description: MozReview Request: Bug 1169044 - Set correct referrer on load from window global. r=bkelly → MozReview Request: Bug 1169044 - Set correct referrer on load from window global. r?bkelly,smaug
Attachment #8612572 - Flags: review?(bugs)
Attachment #8612572 - Flags: review?(bkelly)
Attachment #8612572 - Flags: review+
https://reviewboard.mozilla.org/r/9597/#review8471

::: dom/base/nsContentUtils.cpp:7815
(Diff revision 2)
> +    referrerURI = principalURI;

Please add { } around the if block.

::: dom/base/nsContentUtils.cpp:7786
(Diff revision 2)
> +  sop->GetPrincipal()->GetURI(getter_AddRefs(principalURI));

Is GetPrincipal() guaranteed to return non-nullptr?  I don't see anything in the API guaranteeing this and its a bare pointer deref.

Maybe store it in an nsCOMPtr<> before you access it here and in the IsSystemPrincipal?
Attachment #8612572 - Flags: review?(bkelly) → review+
Comment on attachment 8612572 [details]
MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey

https://reviewboard.mozilla.org/r/9599/#review8477

Ship It!
Comment on attachment 8612572 [details]
MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey

Why Fetch doesn't need to deal with SetReferrerWithPolicy, but XHR and other code does need to?
I don't like this inconsistency.
I'd prefer also that part being moved to the helper method.

Or perhaps there is some reason why Fetch handles this all differently to any other code, but I'm not familiar with Fetch at all. Anyhow, I'd prefer
the helper to deal with SetReferrerWithPolicy at least if one explicitly
passes an httpChannel to the method.
Attachment #8612572 - Flags: review?(bugs) → review-
Attachment #8612572 - Attachment description: MozReview Request: Bug 1169044 - Set correct referrer on load from window global. r?bkelly,smaug → MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?smaug
Attachment #8612572 - Flags: review?(bugs)
Attachment #8612572 - Flags: review-
Attachment #8612572 - Flags: review+
Comment on attachment 8612572 [details]
MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey

Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?smaug
Bug 1169044 - Patch 2 - Split URLSearchParams parsing logic into non-CCed URLParams. r?baku
Attachment #8615684 - Flags: review?(amarchesini)
Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r?bkelly

The ServiceWorkerRegistrationInfo's principal is the principal of the document
that called register(). If we create WorkerPrivate instances based off of
this, they have a valid principal in terms of security and same-origin-ness,
but the URI path is wrong. When fetching the script from the network, the
channel's principal is used to update the worker principal. We need to do the
same when the script is loaded from Cache. This patch adds support to store the
channel principal in the cache.
Attachment #8615685 - Flags: review?(bkelly)
Attached patch Patch 2 bugzilla — — Splinter Review 
Assignee: nobody → nsm.nikhil

        Attachment #8615688 -
        Flags: review?(amarchesini)

    
    

    
  
Ehsan, if you are comfortable with cache code, could you take patch 3?
Flags: needinfo?(ehsan)

    
    

    
  
Comment on attachment 8612572 [details]
MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey

Kyle said he could review some more.

        Attachment #8612572 -
        Flags: review?(bugs) → review?(khuey)

    
    

    
  
Fernando,

Patch 3 in this bug adds an assert that the principal of the channel loading an SW is same-origin to the principal of the registration. Obviously, jar is violating that somehow https://treeherder.mozilla.org/logviewer.html#?job_id=8229547&repo=try
Could you tell me if this requires a relaxed assertion or something? Is the jar trying to load a non-jar resource?
Flags: needinfo?(ferjmoreno)

    
    

    
  
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #24)
> Ehsan, if you are comfortable with cache code, could you take patch 3?

Yes.  Can you please reset the flag to me?  I will review tomorrow.
Flags: needinfo?(ehsan)

        Attachment #8615685 -
        Flags: review?(bkelly) → review?(ehsan)

    
    

    
  
Comment on attachment 8615688 [details] [diff] [review]
Patch 2 bugzilla

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

I prefer to have URLSearchParam inheriting from URLParams instead having a mParams obj. Any reason why you decied to go for this approach?

::: dom/base/URLSearchParams.cpp
@@ +116,3 @@
>  
> +void
> +URLSearchParams::ParseInput(const nsACString& aInput,

move it next to the URLSearchParams ctor.

::: dom/base/URLSearchParams.h
@@ +26,5 @@
>  
>    virtual void URLSearchParamsUpdated(URLSearchParams* aFromThis) = 0;
>  };
>  
> +class URLParams final

What about using this class as base class for URLSearchParams?

@@ +71,5 @@
> +  void Serialize(nsAString& aValue) const;
> +
> +  void Get(const nsAString& aName, nsString& aRetval);
> +
> +  void GetAll(const nsAString& aName, nsTArray<nsString >& aRetval);

I've always wanted to remove this extra space  between nsString and '>'. Can you do it in this patch? :)

        Attachment #8615688 -
        Flags: review?(amarchesini)

    
    

    
  
Comment on attachment 8615685 [details]
MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan

https://reviewboard.mozilla.org/r/10345/#review9089

Ship It!

::: dom/cache/DBSchema.cpp:1747
(Diff revision 1)
> +  }

Are we supposed to parse the contents of the string manually like this?  You may want to check that with bholley.

::: dom/workers/ServiceWorkerScriptCache.cpp:102
(Diff revision 1)
> +                       loadGroup); // FIXME(nsm): TYPE_SERVICEWORKER

Nit: please move the comment back up.

::: dom/workers/ServiceWorkerScriptCache.cpp:470
(Diff revision 1)
> +      return rv;

You Cancel()ed the channel above in the case of an error.  Why not do the same here?

        Attachment #8615685 -
        Flags: review?(ehsan) → review+

    
    

    
  
Comment on attachment 8615686 [details]
MozReview Request: Bug 1169044 - Patch 4 - Set referrer on intercepted request. r=jdm

https://reviewboard.mozilla.org/r/10347/#review9093

Ship It!

        Attachment #8615686 -
        Flags: review?(josh) → review+

    
    

    
  
(In reply to Andrea Marchesini (:baku) from comment #30)
> Comment on attachment 8615688 [details] [diff] [review]
> Patch 2 bugzilla
> 
> Review of attachment 8615688 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I prefer to have URLSearchParam inheriting from URLParams instead having a
> mParams obj. Any reason why you decied to go for this approach?

To me, "URLSearchParams is a webidl object with observer support that *has* a bag of name value pairs" reads better than "Here is this bag of name value pairs, but we are going to specialize it so it also *is* this WebIDL object with observer support". I'd like to keep using composition unless there is a strong reason to use inheritance.

> 
> ::: dom/base/URLSearchParams.cpp
> @@ +116,3 @@
> >  
> > +void
> > +URLSearchParams::ParseInput(const nsACString& aInput,
> 
> move it next to the URLSearchParams ctor.
> 
> ::: dom/base/URLSearchParams.h
> @@ +26,5 @@
> >  
> >    virtual void URLSearchParamsUpdated(URLSearchParams* aFromThis) = 0;
> >  };
> >  
> > +class URLParams final
> 
> What about using this class as base class for URLSearchParams?
> 
> @@ +71,5 @@
> > +  void Serialize(nsAString& aValue) const;
> > +
> > +  void Get(const nsAString& aName, nsString& aRetval);
> > +
> > +  void GetAll(const nsAString& aName, nsTArray<nsString >& aRetval);
> 
> I've always wanted to remove this extra space  between nsString and '>'. Can
> you do it in this patch? :)

    
    

    
  
https://reviewboard.mozilla.org/r/10345/#review9121

> You Cancel()ed the channel above in the case of an error.  Why not do the same here?

`SetPrincipalInfo(nsIChannel*)` is called from OnStartRequest() where returning an error automatically leads to cancel.

    
    

    
  
Comment on attachment 8615685 [details]
MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan

Bobby, is this sort of principal parsing in DBSchema.cpp ok? This is on a background thread with no CC etc, so I can't use any main thread only or webidl helpers.

        Attachment #8615685 -
        Flags: feedback?(bobbyholley)

    
    

    
  
FindChar('!') looks wrong to me, since a URI can contain '!' in it, no?

    
    

    
  
Ah, but the serialization format may not include the ! if it's not present.

That seems broken to me, since it means it can't be unambiguously parsed.  Consider a codebase principal for file:///tmp/foo!appId=5&inBrowser=1

    
    

    
  
Comment on attachment 8615685 [details]
MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan

We shouldn't have any manual parsing logic scattered throughout the tree - anything we do should live with all the other code in CAPS.

        Attachment #8615685 -
        Flags: feedback?(bobbyholley) → feedback-

    
    

    
  
(In reply to Not doing reviews right now from comment #37)
> Ah, but the serialization format may not include the ! if it's not present.

Yes, so that the origins match up with the old behavior when no special OriginAttributes are present.

> That seems broken to me, since it means it can't be unambiguously parsed. 
> Consider a codebase principal for file:///tmp/foo!appId=5&inBrowser=1

Hm, that's a good point. Maybe we should aways include the '!'? What do you think Jonas?
Flags: needinfo?(jonas)

    
    

    
  
Comment on attachment 8612572 [details]
MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey

Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey

        Attachment #8612572 -
        Attachment description: MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?smaug → MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey

    
    

    
  
Comment on attachment 8615684 [details]
MozReview Request: Bug 1169044 - Patch 2 - Split URLSearchParams parsing logic into non-CCed URLParams. r?baku

Bug 1169044 - Patch 2 - Split URLSearchParams parsing logic into non-CCed URLParams. r?baku

        Attachment #8615684 -
        Flags: review?(amarchesini)

    
    

    
  
Comment on attachment 8615685 [details]
MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan

Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan

The ServiceWorkerRegistrationInfo's principal is the principal of the document
that called register(). If we create WorkerPrivate instances based off of
this, they have a valid principal in terms of security and same-origin-ness,
but the URI path is wrong. When fetching the script from the network, the
channel's principal is used to update the worker principal. We need to do the
same when the script is loaded from Cache. This patch adds support to store the
channel principal in the cache.

        Attachment #8615685 -
        Attachment description: MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r?bkelly → MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan

        Attachment #8615685 -
        Flags: review+

        Attachment #8615685 -
        Flags: feedback-

    
    

    
  
Comment on attachment 8615686 [details]
MozReview Request: Bug 1169044 - Patch 4 - Set referrer on intercepted request. r=jdm

Bug 1169044 - Patch 4 - Set referrer on intercepted request. r=jdm

        Attachment #8615686 -
        Attachment description: MozReview Request: Bug 1169044 - Patch 4 - Set referrer on intercepted request. r?jdm → MozReview Request: Bug 1169044 - Patch 4 - Set referrer on intercepted request. r=jdm

        Attachment #8615686 -
        Flags: review+

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #39)
> Hm, that's a good point. Maybe we should aways include the '!'? What do you
> think Jonas?

Happy to leave this to you.

Generally I think it's good if we don't ever need to parse these strings. But clearly they need to always be unique.

Another thing that's tricky is that on one hand we want don't want a filename to be able to collide with the origin suffix.

On the other hand we want to be able to use the origin string as a filename for things like indexedDB...
Flags: needinfo?(jonas)

    
    

    
  
Comment on attachment 8615685 [details]
MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan

Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan

The ServiceWorkerRegistrationInfo's principal is the principal of the document
that called register(). If we create WorkerPrivate instances based off of
this, they have a valid principal in terms of security and same-origin-ness,
but the URI path is wrong. When fetching the script from the network, the
channel's principal is used to update the worker principal. We need to do the
same when the script is loaded from Cache. This patch adds support to store the
channel principal in the cache.

        Attachment #8615685 -
        Flags: review?(bobbyholley)

    
    

    
  
Comment on attachment 8615686 [details]
MozReview Request: Bug 1169044 - Patch 4 - Set referrer on intercepted request. r=jdm

Bug 1169044 - Patch 4 - Set referrer on intercepted request. r=jdm

    
    

    
  
Bobby, only the caps changes.

    
    

    
  
Unrelatedly, I think it would be really great if we didn't have to replicate this "grab the current principal, unless pushState has been called, in that case grab the document URI" code everywhere.

It would be great if we:

1. Added a field for "document URI" to nsIPrincipal. Update this field any time pushState is called.
   (possibly we could even mutate the existing URI on the principal, but that feels somewhat scary.
   Audit would be needed)
2. Add a loadinfo security-flag for explicitly preventing any referrer from being sent.
3. Make the http-channel code get the referrer from loadInfo.triggeringPincipal.documentURI unless
   a different referrer is explicitly set, or unless the above flag has been set.
4. Remove the explicitly set referrer in all/most cases.

    
    

    
  
> Added a field for "document URI" to nsIPrincipal

Multiple documents that have distinct document URIs can share a principal in general, no?  Now if we're doing this for referrers that would be because some cases alias another document's referrer, but...

It's not quite clear to me what should happen if a page has an about:blank or data: or javascript: subframe or a document.opened subframe and then does pushState.  Should the referrer be the one from before or after the pushState?  Note that afaict per spec it should be about:blank or data: or the URI the document it had when he javascript: ran, respectively.  But note https://www.w3.org/Bugs/Public/show_bug.cgi?id=24613

    
    

    
  
(In reply to Not doing reviews right now from comment #50)
> > Added a field for "document URI" to nsIPrincipal
> 
> Multiple documents that have distinct document URIs can share a principal in
> general, no?

file:// aside for a second, can this happen in other cases than when a principal is inherited? I.e. other than for data:, javascript:, about:srcdoc and about:blank?

> https://www.w3.org/Bugs/Public/show_bug.cgi?id=24613

Not much action there the last few months :(

Early on in the bug it did seem like people were fine with "walking up the iframe chain" for cases where we currently inherit.

I just hate the idea of having to pass in a "triggering document" in addition to "triggering principal" any time a channel is created. Though I guess it's always a possibility.

    
    

    
  
Comment on attachment 8615685 [details]
MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan

https://reviewboard.mozilla.org/r/10345/#review9139

r=me on the caps changes with that.

::: caps/BasePrincipal.cpp:122
(Diff revision 3)
> +  int32_t pos = aOrigin.FindChar('!');
> +

Can you make this an rfind, which will make it more robust per bug 1172080?

        Attachment #8615685 -
        Flags: review?(bobbyholley) → review+

    
    

    
  
> file:// aside for a second, can this happen in other cases than when a principal is
> inherited?

Not really.  Possibly add to your list document.open(); I don't recall what that does with principals.

> I just hate the idea of having to pass in a "triggering document" in addition to
> "triggering principal"

So one option here is to go ahead and store the actual origin information in some other data structure pointed to by an principal, so that we can share _that_ across documents but not share the principal pointer itself.  Note that we really need to do something like that anyway to correctly implement hixie's spec's asymmetric handling of document.domain.  Assuming we want to implement that, of course.

    
    

    
  
I think it depends on what design we want. If we want to implement the "walk up the chain to find a referrer" then I think the current design of sharing nsIPrincipal makes sense. Otherwise yeah, we could share something else instead.

So maybe getting that settled in the spec w3c bugzilla thread would be a good idea.

I don't have an opinion about document.domain.

    
    

    
  
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #27)
> Fernando,
> 
> Patch 3 in this bug adds an assert that the principal of the channel loading
> an SW is same-origin to the principal of the registration. Obviously, jar is
> violating that somehow
> https://treeherder.mozilla.org/logviewer.html#?job_id=8229547&repo=try
> Could you tell me if this requires a relaxed assertion or something? Is the
> jar trying to load a non-jar resource?

Sorry, I cannot tell why that assertion is failing :( The JAR channel should only be loading JAR resources. FWIW I did a quick test and it seems that every request from that test goes to a URL like

jar:file:///var/folders/zh/bl0hypv16pb3x474nvy2f3yh0000gp/T/tmpYYGc1w.mozrunner/webapps/%7B2bab62a9-9532-b04b-9a64-79da1d822aac%7D/application.zip!/*

So that seems to be fine.
Flags: needinfo?(ferjmoreno)

    
    

    
  
Andrea, please see comment 33 and reconsider the review of patch 2. Thanks.
Flags: needinfo?(amarchesini)

    
    

    
  
Comment on attachment 8615688 [details] [diff] [review]
Patch 2 bugzilla

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

Ok to have URLSearchParams not inheriting URLParams but in the source code can you split the 2 implementations? In the same file is ok but not mix URLSearchParams and URLParams methods. Thanks.

        Attachment #8615688 -
        Flags: review+

    
    

    
  
Comment on attachment 8615684 [details]
MozReview Request: Bug 1169044 - Patch 2 - Split URLSearchParams parsing logic into non-CCed URLParams. r?baku

https://reviewboard.mozilla.org/r/10343/#review9373

Ship It!

        Attachment #8615684 -
        Flags: review?(amarchesini) → review+

    
    

    
  
Per comment https://bugzilla.mozilla.org/show_bug.cgi?id=1155153#c58, setting v2 dependency here. Thanks!
Blocks: ServiceWorkers-B2G
Flags: needinfo?(amarchesini)

    
    

    
  
Comment on attachment 8624921 [details] [diff] [review]
Patch 6 - wpt expectations update

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

I think you didn't mean to delete the __dir__.ini files; that's a bug in the -update script (which is fixed upstream). Please restore these.

        Attachment #8624921 -
        Flags: review?(james) → review-

        Attachment #8625509 -
        Flags: review?(james) → review+

    
    

    
  
Comment on attachment 8612572 [details]
MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey

https://reviewboard.mozilla.org/r/9599/#review10435

Ship It!
See Also:  → 1178233
See Also:  → 1178236

    
    

    
  
Checking possible android regression as here:
Bug 1179706 - Can't load page https://medium.com/starts-with-a-bang

    
    

    
  
(In reply to Mark Capella [:capella] from comment #77)
> Checking possible android regression as here:
> Bug 1179706 - Can't load page https://medium.com/starts-with-a-bang

Separate issue.  See bug 1178508.  I will be landing a fix as soon as the trees re-open.

    
  
Depends on: 1180765
Depends on: 1224580
Component: DOM → DOM: Core & HTML
Regressions: 1673680

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: