Closed Bug 1385308 Opened 3 years ago Closed 3 years ago

FirstPartyDomain should be properly set for about:newtab


(Firefox :: New Tab Page, defect)

Not set



Firefox 57
Tracking Status
firefox57 --- fixed


(Reporter: ursula, Assigned: allstars.chh)




(1 file, 2 obsolete files)

After giving Activity Stream URI_SAFE_FOR_UNTRUSTED_CONTENT in Bug 1021667, we were forced to exempt about:newtab from the test browser_firstPartyIsolation_aboutPages.js since it was failing with the following error:

69 INFO loading page about:newtab, origin is about:newtab
70 INFO principal [xpconnect wrapped (nsISupports, nsIPrincipal, nsISerializable)]
Buffered messages finished
71 INFO TEST-UNEXPECTED-FAIL | browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js | The about page should have firstPartyDomain set - "" == "about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla" -

We should figure out how to properly set the firstPartyDomain for about:newtab, or at least figure why the test is failing when we set the URI_SAFE_FOR_UNTRUSTED_CONTENT flag in the first place.
Depends on: 1021667
Assignee: nobody → allstars.chh
Attached patch Patch. (obsolete) — Splinter Review
Attachment #8896100 - Flags: review?(bugs)
Attached patch Patch.Splinter Review
fixed eslint error.
Attachment #8896100 - Attachment is obsolete: true
Attachment #8896100 - Flags: review?(bugs)
Attachment #8896123 - Flags: review?(bugs)
Comment on attachment 8896123 [details] [diff] [review]

>+ * In tabbrowser.addTab, when it finds out the uri is about:newtab, it will use
>+ * the preloaded browser, which won't have the pref privact.firstpartyt.isolate

>+ * set.
>+ *
>+ * To prevent to use the preloaded browser, a simple trick is open a window
>+ * first.
>+ */
But doesn't this mean about:newtab doesn't use firstparty.isolate normally?
Isn't there inconsistency that some times pref is set but not always?

Or am I missing something here. Please clarify and ask review again, or fix preloaded browser to have the pref too
Attachment #8896123 - Flags: review?(bugs)
Attached patch Patch. v2 (obsolete) — Splinter Review
Attachment #8896123 - Attachment is obsolete: true
Comment on attachment 8896906 [details] [diff] [review]
Patch. v2

Review of attachment 8896906 [details] [diff] [review]:

Hi smaug
Because the preloaded browser might cause some misunderstanding,
I rewrote the patch with disable preloading browser when the pref 'privacy.firstparty,isolate' is on.
(See commit message for explanation)

Could you help to review this again?
Attachment #8896906 - Flags: review?(bugs)
But why preloading browser works differently? That is really worrisome.
Comment on attachment 8896906 [details] [diff] [review]
Patch. v2

oh, oh, now I see.
I guess the previous patch is ok after all.
Sorry, I somehow missed the issue.
Attachment #8896906 - Flags: review?(bugs)
Attachment #8896123 - Attachment is obsolete: false
Attachment #8896123 - Flags: review+
Pushed by
Test about:newtab should have firstPartyDomain. r=smaug
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1390743
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.