[tor-bugs] #20680 [Applications/Tor Browser]: Rebase Tor Browser patches to 52 ESR

[Tor Bug Tracker & Wiki]

#20680: Rebase Tor Browser patches to 52 ESR
-------------------------------------------------+-------------------------
 Reporter:  arthuredelstein                      |          Owner:  tbb-
                                                 |  team
     Type:  defect                               |         Status:  new
 Priority:  Medium                               |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  ff52-esr, tbb-7.0-must,              |  Actual Points:
  TorBrowserTeam201703, tbb-7.0-must-nightly     |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
                                                 |  Sponsor4
-------------------------------------------------+-------------------------

Comment (by mcs):

 Replying to [comment:36 gk]:
 > `013d0cd1f153626cb7f40cc39288300ee55e100e`: (mcs/brade could you have a
 second look here as well?)
 >
 > in `IsImageExtractionAllowed` why did you replace the old getting-the-
 first-party-code with:
 > {{{
 > +    nsIDocument* topLevelDocument =
 aDocument->GetTopLevelContentDocument();
 > +    nsIURI *topLevelDocURI = topLevelDocument ?
 topLevelDocument->GetDocumentURI() : nullptr;
 > +    nsCString topLevelDocURISpec;
 > +    topLevelDocURI->GetSpec(topLevelDocURISpec);
 > }}}
 > It seems you are not guarding against a possible null-pointer-deref
 there?

 The old API is gone. Kathy and I agree that a NULL pointer check should be
 added.

 > {{{
 > +    rv = permissionManager->TestPermission(docURI,
 > +
 PERMISSION_CANVAS_EXTRACT_DATA, &permission);
 > +    NS_ENSURE_SUCCESS(rv, false);
 > }}}
 > Why not `topLevelDocURI` instead of `docURI`? in 45.8.0 it is
 `firstPartyURI` that gets tested.

 Kathy and I agree that it should be `topLevelDocURI`.

 Here are the additional things that we noticed:
 * The second call to `CanvasPermissionPromptHelper.init();` should be
 `.uninit();`
 * In `browser/base/content/browser.xul`, the class `canvas-icon` was added
 but Kathy and I do not see why it is needed.
 * In `dom/canvas/OffscreenCanvas.cpp`, maybe add a reference to #18599 and
 also mention that for now placeholder image data is always returned to
 users of OffscreenCanvas.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/20680#comment:41>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online