Implement Request.referrerPolicy

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({dev-doc-complete})

unspecified
mozilla47
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Created attachment 8724448 [details] [diff] [review]
Part 1: Implement Request.referrerPolicy
Attachment #8724448 - Flags: review?(josh)
(Assignee)

Comment 2

2 years ago
Created attachment 8724449 [details] [diff] [review]
Part 2: Store the Request referrerPolicy in the DOM Cache

This patch also fixes an existing bug where we used to call
RewriteEntriesSchema at the end of each individual migration,
which is wrong since after the first call, sqlite will think
that it's dealing with the final schema.  So far we were lucky
that we only changed the actual schema once.
Attachment #8724449 - Flags: review?(bkelly)
(Assignee)

Updated

2 years ago
Blocks: 1184549
(Assignee)

Updated

2 years ago
Depends on: 1251873
(Assignee)

Updated

2 years ago
Assignee: nobody → ehsan
(Assignee)

Updated

2 years ago
Keywords: dev-doc-needed
Whiteboard: btpp-active

Comment 3

2 years ago
Comment on attachment 8724449 [details] [diff] [review]
Part 2: Store the Request referrerPolicy in the DOM Cache

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

::: dom/cache/DBSchema.cpp
@@ +104,5 @@
>      "response_principal_info TEXT NOT NULL, "
>      "cache_id INTEGER NOT NULL REFERENCES caches(id) ON DELETE CASCADE, "
>  
>      // New columns must be added at the end of table to migrate and
>      // validate properly.

Can you move this comment to the end?  So its clear to people adding further columns to the table?

@@ +2511,5 @@
>  
> +  // Now overwrite the master SQL for the entries table to remove the column
> +  // default value.  This is also necessary for our Validate() method to
> +  // pass on this database.
> +  rv = RewriteEntriesSchema(aConn);

Why do we want to rewrite the entries schema for all migrations?  This is only needed for the few that need to remove a column default.
Attachment #8724449 - Flags: review?(bkelly) → review+
(Assignee)

Comment 4

2 years ago
(In reply to Ben Kelly [PTO, back Feb 29][:bkelly] from comment #3)
> @@ +2511,5 @@
> >  
> > +  // Now overwrite the master SQL for the entries table to remove the column
> > +  // default value.  This is also necessary for our Validate() method to
> > +  // pass on this database.
> > +  rv = RewriteEntriesSchema(aConn);
> 
> Why do we want to rewrite the entries schema for all migrations?  This is
> only needed for the few that need to remove a column default.

Sure!  Should I set a boolean in the migration function when it needs to rewrite the schema and look at that here to determine whether to call RewriteEntriesSchema?

Comment 5

2 years ago
Is there a reason we can't just call it in the specific migrations that need it like the code currently does?
(Assignee)

Comment 6

2 years ago
(In reply to Ben Kelly [PTO, back Feb 29][:bkelly] from comment #5)
> Is there a reason we can't just call it in the specific migrations that need
> it like the code currently does?

Yes.  If we do what we do today, once you upgrade from a really old version to a newer one so that RewriteEntriesSchema() gets called twice, the first call will update the schema to the *latest* version, not to the version that was the latest at the time that code was written, and in the second place you try to update the schema things will go south since sqlite's notion of the on-disk schema will be different than the actual on-disk schema.

I noticed this when running the test_migration.js js.  That test tries to update a DB from version 10 to the latest.  With my patch applied, the upgrade to version 16 updates the schema to include the request_referrer_policy field in the entries table, and then the ALTER TABLE command in the upgrade to version 20 fails, since it thinks the request_referrer_policy field already exists (even though it doesn't exist in the on-disk contents of the table!)

Sorry if this wasn't obvious.  I tried to explain this in the commit message, but I'm happy to change that description to something more obvious if you have suggestions...

Comment 7

2 years ago
I see.  Ok, maybe just set a boolean somehow that indicates "default column setting needs to be removed" and then do it once at the end.

Thanks for the explanation.

Comment 8

2 years ago
Comment on attachment 8724448 [details] [diff] [review]
Part 1: Implement Request.referrerPolicy

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

::: dom/base/nsContentUtils.h
@@ +2531,5 @@
>     */
>    static nsresult SetFetchReferrerURIWithPolicy(nsIPrincipal* aPrincipal,
>                                                  nsIDocument* aDoc,
> +                                                nsIHttpChannel* aChannel,
> +                                                mozilla::net::ReferrerPolicy aReferrerPolicy = mozilla::net::RP_Default);

Does this need to be a default argument?

::: dom/fetch/FetchDriver.cpp
@@ +308,5 @@
>        NS_ENSURE_SUCCESS(rv, rv);
>  
>        rv =
>          httpChan->SetReferrerWithPolicy(referrerURI,
> +                                        referrerPolicy == ReferrerPolicy::_empty ?

This really belongs in separate variables for clarity; I have a really hard time deciphering nested ternary conditionals.
Attachment #8724448 - Flags: review?(josh) → review+
(Assignee)

Comment 9

2 years ago
(In reply to Josh Matthews [:jdm] from comment #8)
> ::: dom/base/nsContentUtils.h
> @@ +2531,5 @@
> >     */
> >    static nsresult SetFetchReferrerURIWithPolicy(nsIPrincipal* aPrincipal,
> >                                                  nsIDocument* aDoc,
> > +                                                nsIHttpChannel* aChannel,
> > +                                                mozilla::net::ReferrerPolicy aReferrerPolicy = mozilla::net::RP_Default);
> 
> Does this need to be a default argument?

I did that in order to avoid updating the other call sites, but there is only a few of them.  Would you prefer me to make it non-default?
Flags: needinfo?(josh)
(Assignee)

Comment 10

2 years ago
(In reply to :Ehsan Akhgari from comment #9)
> (In reply to Josh Matthews [:jdm] from comment #8)
> > ::: dom/base/nsContentUtils.h
> > @@ +2531,5 @@
> > >     */
> > >    static nsresult SetFetchReferrerURIWithPolicy(nsIPrincipal* aPrincipal,
> > >                                                  nsIDocument* aDoc,
> > > +                                                nsIHttpChannel* aChannel,
> > > +                                                mozilla::net::ReferrerPolicy aReferrerPolicy = mozilla::net::RP_Default);
> > 
> > Does this need to be a default argument?
> 
> I did that in order to avoid updating the other call sites, but there is
> only a few of them.  Would you prefer me to make it non-default?

I'll just do this in order to be able to land this today...

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a87e26a12a98
https://hg.mozilla.org/mozilla-central/rev/4d0d6bb9abee
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Updated

2 years ago
Flags: needinfo?(josh)

Updated

2 years ago
Depends on: 1285502
You need to log in before you can comment on or make changes to this bug.